Replace pako with native DecompressionStream - #113
Conversation
The HTTP-Redirect binding's raw-DEFLATE inflation is the only thing pako was
used for, and native DecompressionStream('deflate-raw') covers it well below
the extension's strict_min_version of 140.0. Removing it drops one of the two
third-party libraries AMO flagged for provenance in issue simplesamlphp#109.
DecompressionStream is async, so b64inflate() and parseSAML() become async.
parseSAML() moves out of the Request constructor, since RequestItem inspects
saml/samlart to decide which tabs exist, and both callers of addRequestItem()
now await it -- including the import path, where restoreFromImport()'s result
was previously assigned to a variable that would have become a
permanently-truthy Promise.
Adds the Playwright suite along with a spec that drives a genuine deflated
SAMLRequest end-to-end, plus jest coverage of the inflate itself including
latin1 byte semantics and Shibboleth's line-wrapped base64.
…als from new contexts
|
Wow, that's really impressive! |
|
I just wanted to create a release for v1.9.5. Unfortunately I short on time (as always :-( ) and the build fails: Any ideas why? |
|
Thanks, that's very kind! I use SAML-tracer daily (hourly?) at UVA so I'm happy to help contribute! On the build failure: I don't think it's related to this PR, it's failing in
- uses: actions/checkout@v6
with:
token: ${{ secrets.SAMLTRACER_BUILD_TOKEN }}Seems like the token is empty so checkout is falling back to an unauthenticated fetch, which then tries to prompt for credentials and fails. Looks like |
|
Probably expired, so I created a new token and now the build passes! |
|
@khlr We very much depend on you for publication in the app-stores.. Perhaps it's a good idea if some of us could also create new releases, so we don't have to rely on you when you're so busy? |
|
Thank you for fixing the build, @tvdijen ! My role at AMO is just "developer". @jaimeperez and @thijskh are the owners and onboarded me. I think one of them could add you too, Tim. |
|
Jaime is no longer involved, so I guess @thijskh has to add me then.. Doesn't have to be me per se, but Thijs has a busy job too so I think it's a good idea to shuffle the cards so we don't have to rely on you for new releases |
3 participants
This is the pako half of the plan discussed in #109. pako was only ever used in one place —
b64inflate()in SAMLTrace.js, inflating the HTTP-Redirect binding — and the browser's nativeDecompressionStream('deflate-raw')does the same job, so the library can go away entirely, along withlib/pako_inflate.min.js, its copy step in the release workflow, and its section in attribution.md. One less third-party file for an AMO reviewer to verify.The one wrinkle is that DecompressionStream is asynchronous where pako was synchronous, so
parseSAML()moves out of the Request constructor and is awaited inaddRequestItem()before the request is published to the list, and the import path in SAMLTraceIO.js awaits it likewise. The parameter-matching loop keeps its first-match-wins semantics, just written as a sequentialfor...ofinstead ofArray.some(). The manifest already requires Firefox 140, comfortably above wheredeflate-rawsupport landed.On tests: a new jest suite pins the
b64inflate()behaviour, including the subtle detail thatpako.inflateRawplusString.fromCharCodeproduced a latin1 byte-string rather than decoded UTF-8 — downstream parsing depends on that, so the replacement is asserted to match it byte for byte. There is also a Playwright end-to-end suite (npm run test:e2e, documented in test/E2E_TESTS.md) that loads the extension in a real Chromium and drives a genuine raw-deflated SAMLRequest through the query string. That spec is implementation-agnostic and passes against pako too, so it serves as a true before/after check for this change. The suite needs a display and reaches example.com over the network, so I deliberately did not wire it into CI — happy to add a workflow for it if you want one, but that seemed like your call to make. Two of its specs cover the #69/#106 export fixes from #108 rather than this change; they ride along because they share the harness and those fixes previously had no browser-level coverage.The second commit fixes a CI failure I hit on my fork: under jest on Node 26, a context created with
vm.runInNewContextno longer sees web globals likeatobandDecompressionStream, so the new test loads SAMLTrace.js through aFunctionwrapper in the test's own scope instead. SAMLTraceIO.test.js keeps the vm loader unchanged, since it touches no web globals.The branch is rebased onto main after #112. Jest is green on Node 26 in CI, and the Playwright suite passes 5/5 locally against the combined pako-removal and cdn-assets state.