Set inResponseTo validation to always#399
Conversation
WalkthroughThe default for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/tests.spec.ts (1)
3000-3007: Missing bulk updates – most tests still assumeValidateInResponseTo.neverOnly this single test explicitly resets
validateInResponseTotonever, but the build log shows dozens of failures where other test-suites still rely on the old default.
Either:
- Pass
validateInResponseTo: ValidateInResponseTo.neverin every test that doesn’t care about InResponseTo semantics, or- Keep the stricter default (
always) and update each assertion to expect the newInResponseTo-related errors.Right now we have a half-way state: two tests patched, >25 still broken.
CI will stay red until this is addressed project-wide.
🧹 Nitpick comments (1)
test/tests.spec.ts (1)
3028-3032: Consider helper for repetitive SAML config overridesMultiple tests will now need the same override:
{ ...baseConfig, validateInResponseTo: ValidateInResponseTo.never }To avoid copy-paste and future drift, wrap it in a small factory/helper (e.g.
createTestSaml(configOverrides)).
Keeps the suites concise and guarantees consistency when defaults change again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/saml.ts(1 hunks)test/tests.spec.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build Status
src/saml.ts
[error] 91-91: SAML.validateInResponseTo method is causing multiple test failures with 'InResponseTo is not valid' errors.
test/tests.spec.ts
[error] 2453-2903: Assertion condition checks failed with 'InResponseTo is not valid' errors in various tests related to SAML assertion validation.
[error] 2517-2707: Assertion failures due to 'InResponseTo is not valid' replacing expected date parsing errors in assertion condition checks.
[error] 777-837: validatePostResponse tests failed with 'InResponseTo is not valid' errors instead of expected SAML error messages.
[error] 1104-1185: validatePostResponse xml signature checks failed with 'InResponseTo is not valid' errors replacing expected audience validation errors.
[error] 1245-1543: Multiple validatePostResponse xml signature tests failed with 'InResponseTo is not valid' or 'InResponseTo is missing from response' errors.
[error] 1168-1168: Audience mismatch test failed with 'InResponseTo is not valid' error instead of expected audience mismatch error.
[error] 1287-1418: Tests involving duplicate altered assertions and idpCert function validations failed due to 'InResponseTo is not valid' errors.
[error] 1507-1507: XML AttributeValue without signature test failed with 'InResponseTo is missing from response' error instead of 'Invalid signature'.
[error] 1318-1320: Tests for extra nested and altered assertions failed with 'InResponseTo is not valid' errors instead of expected invalid signature errors.
[error] 2658-2681: Assertion condition checks for expired and max assertion age failed with 'InResponseTo is not valid' errors.
[error] 2536-2536: Assertion check for NotOnOrAfter time failed with 'InResponseTo is not valid' error instead of expected expiration error.
[error] 2492-2492: Assertion check for NotBefore time failed with 'InResponseTo is not valid' error instead of expected 'not yet valid' error.
[error] 2801-2801: Assertion check for audience mismatch failed with 'InResponseTo is not valid' error instead of expected audience mismatch error.
[error] 777-815: validatePostResponse tests for error status codes and NoPassive status failed with 'InResponseTo is not valid' errors.
[error] 837-837: validatePostResponse test for attributeStatement without attributeValue failed with 'InResponseTo is not valid' error.
[error] 1364-1389: validatePostResponse xml signature checks for idpCert function validations failed with 'InResponseTo is not valid' errors.
[error] 1220-1271: validatePostResponse xml signature checks for valid and altered assertions failed with 'InResponseTo is not valid' errors.
[error] 52-52: Multiple tests failed due to assertion errors where 'InResponseTo is not valid' replaced expected signature validation messages.
🔇 Additional comments (1)
README.md (1)
118-119: Documentation accurately reflects the code changes.The swap of descriptions and default annotation correctly documents that
"always"is now the default behavior forvalidateInResponseTo, matching the code change insrc/saml.ts.
| "urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport", | ||
| ], | ||
| validateInResponseTo: ctorOptions.validateInResponseTo ?? ValidateInResponseTo.never, | ||
| validateInResponseTo: ctorOptions.validateInResponseTo ?? ValidateInResponseTo.always, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security improvement: Good change to enable InResponseTo validation by default.
This change tightens security by making InResponseTo validation the default behavior, which helps prevent replay attacks and ensures proper request-response correlation.
However, the pipeline failures indicate test issues with this change. Please verify that all tests have been properly updated to handle the new default behavior:
🏁 Script executed:
#!/bin/bash
# Description: Check for tests that might need explicit validateInResponseTo: never configuration
# Search for test files that might be affected by the validateInResponseTo default change
rg -A 10 -B 5 "validateInResponseTo.*never" test/
rg -A 10 -B 5 "InResponseTo.*not.*valid" test/
rg -A 10 -B 5 "new SAML\(" test/ | head -20Length of output: 23912
Fix test failures by specifying validateInResponseTo explicitly in tests
The default for validateInResponseTo was tightened from never to always for security, which is correct. However, many existing tests assume the old default and now fail. Please update your test suites to explicitly opt-out of InResponseTo validation where needed:
• test/tests.spec.ts
– Around each validateInResponseTo: ValidateInResponseTo.never instantiation, ensure the SAML constructor includes
validateInResponseTo: ValidateInResponseTo.never.
• test/test-signatures.spec.ts
– In the new SAML({ … }) calls for signature tests that shouldn’t enforce InResponseTo, add
validateInResponseTo: ValidateInResponseTo.never.
• test/samlTests.spec.ts (and any other SAML-related spec)
– For cases “not problematic if not validated”, set
validateInResponseTo: ValidateInResponseTo.never.
Example update:
- const samlObj = new SAML({
- callbackUrl: "http://localhost/saml/consume",
- idpCert,
- issuer: "onesaml_login",
- audience: false,
- // …other options
- });
+ const samlObj = new SAML({
+ callbackUrl: "http://localhost/saml/consume",
+ idpCert,
+ issuer: "onesaml_login",
+ audience: false,
+ validateInResponseTo: ValidateInResponseTo.never, // explicit override for this test
+ // …other options
+ });After these adjustments, rerun the pipeline to confirm all tests pass.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/saml.ts at line 137, the default for validateInResponseTo was changed
from never to always, causing test failures. To fix this, update all relevant
test files—test/tests.spec.ts, test/test-signatures.spec.ts, and
test/samlTests.spec.ts—by explicitly setting validateInResponseTo:
ValidateInResponseTo.never in the SAML constructor options where InResponseTo
validation should be disabled. After making these changes, rerun the tests to
ensure they pass.
inResponseTo validation to always
markstos
left a comment
There was a problem hiding this comment.
The new default ffollows the philosophy of "secure by default".
|
Attackers can just exclude inResponseTo (and there's no mechanism to reject SAML responses without InResponseTo). So this doesn't really make it more secure. |
|
With this default, the setting of |
|
I checked the code, and yes @cjbarth you are correct. However, many applications use IdP initiated SSO, which has SAML responses without InResponseTo. I agree that this has some security benefit, namely preventing login CSRF. When you click the Okta/Entra app tiles and the application logs in automatically you are basically using IdP initiated SSO. How many applications you use support this flow? Your change would basically break this flow. I would instead change it to ifPresent, and then add some documentation saying the risks of IdP initiated SSO/login CSRF, and recommend changing it to always |
Description
This tightens
inResponseTovalidation toalwaysbased on this comment.Summary by CodeRabbit
Documentation
validateInResponseTosetting.Bug Fixes
InResponseTovalidation where necessary, ensuring consistent test results.Refactor
InResponseToattribute in SAML responses unless specified otherwise.