Repository navigation
fix: security hardening - #1117
Merged
Merged
Conversation
checkStatusCode reported the error through the callback but did not stop the caller, so validation carried on after the promise had already rejected: signature checks ran and, if they passed, the caller's assertionReplayValidator was invoked and could record the assertion as used even though the response was rejected. Replace it with getStatusCodeError, which returns the error, and return right after reporting it in both parseInternal and validateInternal. Tests: add a regression test that a signed assertion inside a non-Success Response never reaches the replay validator, and tighten the two existing StatusCode tests to assert.rejects (the try/catch versions passed even when nothing threw). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
validateAudience read Conditions.AudienceRestriction.Audience as a single value. With more than one <Audience> the parsed value is an array, audience.startsWith threw, and the catch returned false, so a valid assertion naming the SP among several audiences was always rejected, even with strictAudienceValidation. Follow SAML core 2.5.1.4: every <AudienceRestriction> must be satisfied, and within one restriction any <Audience> may match. The expected audience can still be a string or a list, and the prefix/strict matching rule is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
parseLogoutResponse read Issuer[0]._, which xml2js only sets when the element has attributes, so <saml:Issuer>https://idp</saml:Issuer> came back as issuer: undefined. A document with a different root element, or without a StatusCode, threw a raw TypeError. Add a textOf helper that handles both xml2js shapes, use it for the Issuer, and reject with clear errors when the LogoutResponse element or its StatusCode is missing. parseLogoutRequest's three copies of the same string-or-object logic now use the helper too. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- The nameIDFormat error read validateOpts.nameIDFormat, which callers never set, so it said "...set 'Name ID Format' to undefined". It now names the expected validateNameIDFormat value. The existing test had locked in the "undefined" text; it is updated. - A descriptor without any <KeyDescriptor> threw "keyDes is not iterable". It is now treated as having no keys, so the caller gets "Could not find X509Certificate in the IdP metadata." - Return after rejecting for a missing certificate instead of carrying on to build and resolve a result that is then ignored. - Merge the three separate imports from ./utils. The two affected tests now use assert.rejects so they fail when nothing is thrown. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- A document whose root is not AuthnRequest threw "Cannot read properties of undefined (reading '$')". It now rejects with "Invalid SAML Request: missing AuthnRequest element." - A <Signature> without KeyInfo/X509Data/X509Certificate threw a raw TypeError from the hard-coded [0] chain. A Signature with no certificate is now treated like no Signature: publicKey is null, and the POST binding rejects with "Missing signature" as before. - An X509Certificate element that carries attributes now yields its text rather than the xml2js object. - parseXML returns after rejecting instead of also calling resolve. Tests cover these cases plus the previously untested "Missing Issuer" and "Missing signature" paths. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
getVersion read assertion['@'].Version, which throws when the Assertion element has no attributes. It runs inside the callback chain of parse()/validate(), so the TypeError escaped as an unhandled rejection and the returned promise never settled: parse() on such a response hung. Use optional chaining so getVersion returns null, and the caller rejects with "SAML Assertion version not supported." as intended. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
PubKeyInfo emitted </X509Certificate</X509Data>, missing a '>'. Signed output happened to come out well-formed only because xmldom repaired the fragment when xml-crypto parsed it. The existing test stripped that exact malformed string, so it passed with the bug; it now compares the full output, and new tests cover the namespace prefix and parse the result as XML. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
createIdPMetadataXML strips -----BEGIN/END CERTIFICATE----- before writing <X509Certificate>, but createSPMetadataXML wrote publicKeyString as given, so a PEM input put the header and footer inside the element. An IdP reading that metadata gets an invalid certificate, and our own parseMetadata computed a wrong thumbprint from it. Strip them the same way for both the signing and encryption KeyDescriptors. The new test round-trips a PEM certificate through createSPMetadataXML and parseMetadata and checks the key and thumbprint. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
xml-encryption calls the decrypt callback inside its own try/catch, and feeds anything the callback throws back into the same callback as a decryption error. Our callback both threw on error and did the follow-up work (parsing the plaintext, swapping it into the document), so: - every failure, including a multi-rooted decrypted payload, surfaced as the generic "Exception of Assertion Decryption." with the cause lost, so validate() could not map it to the multi-root error; - the callback ran a second time after the first throw. The callback now only records err/result; the follow-up work runs after xmlenc.decrypt returns, so its errors reach the caller unchanged. A real decryption failure is a WrapError with the xml-encryption error in `inner`. WrapError moves to utils.ts so decrypt.ts can use it without importing response.ts (which imports decrypt.ts); response.ts re-exports it, so the public export is unchanged. The insecure-algorithm setting is untouched here. Tests encrypt fixtures with the testIdp key pair to cover a clean decrypt and a multi-rooted payload through decryptXml and validate(), and the existing error tests now use assert.throws. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Add test/helpers/fixtures.ts with builders for assertions and Responses that have a fresh validity window, plus helpers to encrypt and sign them with the testIdp key pair. It sits outside test/lib so mocha does not load it as a spec. New tests: - validate() on a Response signed over its <EncryptedAssertion> (the OpenAthens-style fallback, previously untested): accepted with the decrypted profile; rejected when tampered, when the signing cert is not trusted, and when no decryption key is given. - parse(): a Response without an Assertion, a Response with several Assertions (the first is used), and the wrapped error when the attribute parser throws. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- A document without any Signature is rejected, with and without an explicit maxSignatureReferences. - A Reference without a DigestMethod, and a SignedInfo without any Reference, are rejected (xml-crypto refuses both while loading the signature, before our own DigestMethod allowlist check runs). - A Signature nested deeper than depth 4 is found by the fallback lookup, verifies, and stops verifying once the signed content changes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- parseLogoutRequest: a wrong root element, the certificate embedded in Signature/KeyInfo (plain and with attributes), a Signature without a certificate, a request with only the required attributes, and an Extensions Attribute that is not id_token. - createLogoutRequest and createLogoutResponse output now parse back to the values they were built from. The LogoutResponse round trip is an end-to-end regression test for the plain-text Issuer fix: it fails on the original logout.ts with issuer: undefined. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
saml20.ts: - validateExpiration: an unparseable NotBefore, a NotBefore just inside and just outside the 10-minute skew, and a bearer window that has not started. - validateRecipient: no SubjectConfirmation (only a signed Destination can match), a non-bearer confirmation's Recipient is ignored, and a bearer confirmation without SubjectConfirmationData is skipped. - getNotOnOrAfter: SubjectConfirmationData without a bound is ignored, the result is the earlier of Conditions and the latest bearer bound, and no bound gives undefined. - validateAudience: a non-string expected audience returns false. utils.ts parseFromString: the fixed "Invalid XML." for non-fatal parser errors (trailing text, undeclared entity), a text-only document, and a root surrounded by comments and whitespace. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
The audience fix was covered with hand-built objects only. These tests sign real Responses and run them through validate() with strict audience matching: the SP listed among several audiences is accepted, no matching audience is rejected, and every AudienceRestriction must name the SP. Two of the three fail on the original saml20.ts. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- createPostForm: values are HTML-escaped, characters in the action URL that would break the attribute are percent-encoded, no params (empty or undefined) renders just the submit button, and an empty value stays empty. - sign(): the output uses RSA-SHA256 with SHA-256 digests, places the Signature right after the Issuer, verifies against the configured certificate and, through the thumbprint path, against the certificate embedded in KeyInfo; it stops verifying once the content changes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- test/mocha.opts: mocha stopped reading it in v8, and its two settings (spec reporter, bdd ui) are the defaults anyway. - tsconfig-paths: not referenced anywhere; tsx resolves paths itself. - tsconfig experimentalDecorators and resolveJsonModule: the code uses neither decorators nor JSON imports. The compiled dist/ output is byte-identical before and after. - package.json readmeFilename: an npm-generated field with no effect in a source package.json. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
The library runs on Node, not in a browser, so ESLint should know `process`, `Buffer` and friends rather than `window` and `document`. Test files additionally get mocha's describe/it/before globals. For .ts files typescript-eslint already turns no-undef off, so today this only affects plain JS files; the warning count is unchanged (22). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
tsconfig.json builds dist/ from lib/ only and excludes test/, and tsx strips types without checking them, so type errors in the tests were never reported. Add tsconfig.test.json (extends tsconfig.json, includes lib/ and test/, no emit), an `npm run typecheck` script, and a CI step after the build. It names the mocha and node type packages explicitly because TypeScript 6 no longer loads every @types package by default. Prettier formats the new file as JSONC, like tsconfig.json. The current tests type-check cleanly. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
- `npm ci` installs exactly what package-lock.json records and fails if it is out of sync with package.json; `npm i` could silently resolve different versions on CI than the ones reviewed in the lockfile. - actions/setup-node no longer has an `always-auth` input (it is not in its action.yml), so the line only produced an "Unexpected input" warning. Publishing authenticates through registry-url and OIDC. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Set "node": ">=16", the floor of the strictest runtime dependency (xml-crypto 6 declares >=16; the library itself also relies on crypto.X509Certificate, available since Node 15.6). This is a declaration, not a tested guarantee: CI only runs Node 24. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Align with Polis, the main consumer, instead of the xml-crypto floor of 16. Node 16, 18 and 20 are past end of life, and CI does not test them. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
npm copies the root package's engines into the lockfile; the commits that added and raised engines did not include the regenerated lock. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
The Reference ceiling (CWE-407) counted SignedInfo/Reference with the XPath "./*[local-name(.)='SignedInfo']/*[local-name(.)='Reference']". In the xpath library that query is quadratic in the number of matches: about 0.7 s for 1000 References, 3 s for 2000 and 14 s for 4000, all before the ceiling could reject the document. A SAMLResponse of roughly 1.5 MB could therefore pin a CPU for seconds per request, which is the very cost the ceiling exists to prevent. Count by walking the DOM children instead. It matches the same nodes (local name only, direct SignedInfo children only) and rejects a 4000-Reference document in about 0.25 s, nearly all of it XML parsing. Tests: a timing regression test (4000 References must be rejected in under 3 s; it took 13 s before) and a test that References inside an unsigned ds:Object/Manifest do not count toward the ceiling. This also fixes the flaky "inflated SignedInfo/Reference list" test, which took 2.2-2.5 s against mocha's 2 s timeout and now takes about 0.5 s. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
validateSignature on a document without a ds:Signature passed undefined to xml-crypto's loadSignature, which failed with "Cannot read properties of undefined (reading 'toString')". validate() wrapped that as "Invalid assertion.", but the inner error and any direct caller of validateSignature saw the raw TypeError. Throw "invalid signature: no Signature element found" before loading the signature, in every path (single cert, multiple certs, thumbprint), and drop the now-unreachable early return in the Reference ceiling check. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
With a thumbprint configured, getCertFromKeyInfo took the first X509Certificate anywhere in the document (via ownerDocument), not the one in the KeyInfo of the signature being verified. This was not a bypass, since the selected key still had to verify the signature, but: - a validly signed assertion was rejected when another certificate appeared earlier in the document (unsigned Extensions, another signature's KeyInfo); - only the first certificate was considered, so a KeyInfo carrying a chain with the trusted certificate second was rejected; - a signature without KeyInfo failed with a TypeError. Search only this KeyInfo, accept the first certificate whose thumbprint is trusted, and return null when there is none, which makes xml-crypto report "KeyInfo or publicCert or privateKey is required to validate signature". The configured-certificate path is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
The form submits itself on load, and its action URL usually comes from SP or IdP metadata, which a tenant can control. A `javascript:` (or `data:`/`vbscript:`) action would run script in the page's origin as soon as the form submits; encodeURI does not stop that. - Parse postUrl with the WHATWG URL parser, as a browser would (it also undoes disguises such as leading spaces or a tab inside "java\tscript"), and throw unless the scheme is http or https. Relative and protocol-relative URLs resolve against an http base, so they keep working. - HTML-escape the hidden inputs' name attribute as well as the value. Callers normally pass fixed names (SAMLResponse, RelayState), so this is hardening. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
|
Warning Review limit reachedYou'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 59 minutes. View limit details
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 |
deepakprabhakara
added this pull request to stack #1116
October 11, 2026 21:46
Validation accepts several AudienceRestriction elements and several Audience values per restriction, but profile parsing still read Conditions.AudienceRestriction.Audience as a single value: - with two restrictions, validate() succeeded and returned profile.audience === undefined; - with several Audience values in one restriction, profile.audience was an array, although SAMLProfile declared it a string. Profile parsing and validateAudience now share one helper that reads the Audience values of every restriction. The profile reports: - audience: the first Audience in document order, or undefined when the assertion names none (the type is now `audience?: string`, which is what it already could be); - audiences: every Audience of every restriction, in document order. The signed-response regression tests now assert the returned audience and audiences, including the two-restriction case, and new parse tests cover one value, several values, several restrictions and none. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
engines was ">=22", but Polis, the main consumer, declares "^20.19.0 || >=22.12.0" and still supports Node 20. Installing this package there on Node 20 would warn with EBADENGINE, or fail where engine-strict is set. Use the same range as Polis. Checked on Node 20.19.0: npm ci, the full test suite, build, typecheck and lint all pass, and the built package loads. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
…heme The action URL was checked for an http(s) scheme, then written into the attribute with encodeURI only. encodeURI leaves `&`, `#` and `;` alone, so `javascript:alert(1)` passed the check as a relative URL, and the HTML parser decoded `:` to `:` when reading the attribute, giving the auto-submitting form a `javascript:` action. Hex, zero-padded and named references (`:`, `:`, `:`), an encoded `j` and a `	` inside the scheme did the same. - The action is now escapeHtml(encodeURI(postUrl)), so HTML decoding yields exactly encodeURI(postUrl). - That encoded value is also checked for an http(s) scheme, so the URL checked is the URL submitted to; the existing check on the raw URL stays. - A URL that encodeURI cannot encode (a lone surrogate) is rejected with "Invalid postUrl: not a valid URL." instead of a raw URIError. A caller that passed an already HTML-escaped URL (`&` in the query) now gets the literal `&` in the submitted URL; URLs are expected unescaped. Tests parse the generated HTML with xmldom's HTML parser, which decodes character references the way a browser does, and check for each tricky input that the form is either rejected or its parsed action equals encodeURI(postUrl) and resolves to http or https. The two tests that compared the raw attribute text now compare the parsed action. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
Collaborator
Author
|
No new findings in 988b8ec on fix/security-hardening. The previous P2 is resolved: HTML escaping preserves entity-containing URLs as literal text, and the new tests verify the parsed action. |
createPostForm already writes escapeHtml(encodeURI(postUrl)) into the action attribute. Two more checks: - The "percent-encodes characters" test also asserts the raw attribute text, `&c=`, not only the value an HTML parser reads back. - Query text that looks like a named character reference (`©`, `©=`, `<...>`) reaches the parsed action unchanged instead of being decoded to a different URL (`©` would become `©`). Both fail on the version of post.ts before the escaping fix. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_015yAnU9PfPVJhyuYqdbhrdx
# Conflicts: # package-lock.json # package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.