Skip to content

Conversation

@NutharaNR
Copy link
Contributor

@NutharaNR NutharaNR commented Jan 14, 2026

Purpose

SAML artifact is being double–URL encoded, even though the SAML specification requires only a single URL encoding[1]. To address this, a new configuration option, saml.artifact.disable_double_encoding, has been introduced. Its default value in older versions(7.1.0 & 7.2.0) is false and in master the value is true, which enforces the correct (single-encoding) behavior.

Related PRs

Related Issues

Test environment

Security checks

Summary by CodeRabbit

  • Documentation
    • Updated SAML authentication guide with deployment configuration guidance for versions 7.1.0 and 7.2.0, including TOML configuration examples for artifact handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@NutharaNR NutharaNR changed the title Add disable_double_encodign config info Add disable_double_encoding config info Jan 14, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

Documentation update adding conditional informational blocks to SAML artifact binding guide that explain disabling double encoding in deployment.toml for product versions 7.1.0 and 7.2.0, with accompanying configuration examples.

Changes

Cohort / File(s) Summary
SAML Documentation
en/includes/guides/authentication/saml/saml-artifact-binding.md
Added 11 lines of version-conditional guidance (7.1.0 and 7.2.0) explaining how to disable SAML artifact double encoding in deployment.toml; includes TOML configuration snippet inserted at two locations within the document

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A guide now clearer, version-aware,
Double encoding tips laid bare,
For 7.1 and 7.2 to see,
Configuration made easy as can be! 📚✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add disable_double_encoding config info' directly describes the main change—adding configuration information about disabling double encoding of SAML artifacts.
Description check ✅ Passed The description includes Purpose, Related PRs, and Related Issues sections as required, though Test environment and Security checks items are incomplete or unchecked.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@en/includes/guides/authentication/saml/saml-artifact-binding.md`:
- Line 213: The MkDocs admonition type in the documentation uses an uppercase
identifier; change the admonition heading `Info` to lowercase `info` (i.e.,
replace `!!! Info` with `!!! info`) so MkDocs Material will recognize and render
the admonition correctly in saml-artifact-binding.md.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 339953d and f63a457.

📒 Files selected for processing (1)
  • en/includes/guides/authentication/saml/saml-artifact-binding.md
🔇 Additional comments (2)
en/includes/guides/authentication/saml/saml-artifact-binding.md (2)

212-221: Documentation accurately reflects the version-specific behavior.

The conditional block correctly targets versions 7.1.0 and 7.2.0 where double encoding is the default behavior, and provides clear guidance on how to disable it with the appropriate configuration. The TOML snippet is properly formatted.


212-221: AI summary incorrectly claimed two locations; only one instance exists.

The search confirms only a single instance of the disable_double_encoding configuration block exists in the document (line 219). The AI-generated summary stating two locations was inaccurate. The current code placement is correct and complete.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

If your are using a different URL, add that as an allowed origin.
{% if is_version == "7.1.0" or is_version == "7.2.0" %}
!!! Info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use lowercase for MkDocs admonition type.

MkDocs Material requires lowercase admonition identifiers. Change Info to info for proper rendering.

📝 Proposed fix
-    !!! Info
+    !!! info
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
!!! Info
!!! info
🤖 Prompt for AI Agents
In `@en/includes/guides/authentication/saml/saml-artifact-binding.md` at line 213,
The MkDocs admonition type in the documentation uses an uppercase identifier;
change the admonition heading `Info` to lowercase `info` (i.e., replace `!!!
Info` with `!!! info`) so MkDocs Material will recognize and render the
admonition correctly in saml-artifact-binding.md.

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.

1 participant