ci: removed obsolete directives... #12962
Conversation
ganesh-at-wiw
requested review from
devrimyatar,
iromli,
yurem,
yuriyz and
yuriyzz
as code owners
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughRemoved obsolete Apache directives and updated legacy access control syntax to "Require all granted" in the HTTPS Apache template. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ 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. Comment |
|
Error: Hi @ganesh-at-wiw, You did not reference an open issue in your PR. I attempted to create an issue for you. |
mo-auto
added
area-CI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-linux-setup/jans_setup/templates/apache/https_jans.conf (1)
90-97: This LocationMatch is allowed by VirtualHost-level access control; clarify whether explicitRequireis needed for consistency.The
<LocationMatch /jans-auth/auth/cert/cert-login.htm>block doesn't have an explicitRequiredirective, unlike all other<Location>blocks in the configuration. However, the VirtualHost *:443 block hasRequire all grantedat the top level, which applies to all nested locations by inheritance—so access is permitted.Since all sibling Location blocks explicitly declare
Require all granted, consider either:
- Adding
Require all grantedhere as well for consistency, or- Documenting why this LocationMatch intentionally relies on VirtualHost-level access control while Java-level authentication (via CertService) handles the actual certificate validation and authorization.
🤖 Fix all issues with AI agents
In @jans-linux-setup/jans_setup/templates/apache/https_jans.conf:
- Line 56: Several "Require all granted" directives are indented with tabs while
the rest of the file uses spaces; replace the leading tab characters before each
"Require all granted" (the occurrences of the directive) with the same number of
spaces used by surrounding blocks so indentation matches the file's existing
style (apply the same change to all instances including the ones noted in the
review).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jans-linux-setup/jans_setup/templates/apache/https_jans.conf
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:0-0
Timestamp: 2025-12-31T16:10:36.685Z
Learning: For the cert-authn plugin in jans-casa, Apache configuration templates (such as certauthn_vhost_tls1.3.conf) are documented in the plugin documentation page rather than with inline comments in the configuration files themselves.
📚 Learning: 2025-12-31T16:10:36.685Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:0-0
Timestamp: 2025-12-31T16:10:36.685Z
Learning: For the cert-authn plugin in jans-casa, Apache configuration templates (such as certauthn_vhost_tls1.3.conf) are documented in the plugin documentation page rather than with inline comments in the configuration files themselves.
Applied to files:
jans-linux-setup/jans_setup/templates/apache/https_jans.conf
🔇 Additional comments (2)
jans-linux-setup/jans_setup/templates/apache/https_jans.conf (2)
54-57: LGTM: Access control modernization is correct.The update from legacy
Order deny,allow/Allow from allsyntax toRequire all grantedis the correct approach for Apache 2.4+. All proxy and location blocks are consistently updated, which is appropriate for a reverse proxy configuration where authentication and authorization are handled by the backend services.Also applies to: 59-63, 65-69, 71-76, 78-82, 84-88, 99-102
27-32: Approve removal of obsolete security directives.The removal of the
X-Xss-Protectionheader and theSetEnvIf User-Agentworkaround for MSIE are appropriate modernization steps. The X-XSS-Protection header is deprecated by OWASP, MDN, and Mozilla standards in favor of Content-Security-Policy, and the MSIE workaround is obsolete since Internet Explorer is no longer supported. The existing security headers (X-Content-Type-Options, HSTS) and CSP infrastructure remain appropriate.Before merging, verify that:
- Your security policy or documentation doesn't explicitly require the X-XSS-Protection header
- No legacy clients depend on the MSIE workaround
jgomer2001
left a comment
jgomer2001
left a comment
There was a problem hiding this comment.
I don't understand why Require replaces the existing Order/Allow. Those are unrelated afaik
Update https_jans.conf Fixed indentation Signed-off-by: Ganesh Dutt Sharma <ganesh@gluu.org>
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-linux-setup/jans_setup/templates/apache/https_jans.conf (1)
17-18: Consider adding TLS 1.3 support, with deployment considerations.The current configuration supports TLS 1.2 only. TLS 1.3 offers improved performance and security, but requires Apache 2.4.38+ with OpenSSL 1.1.1+ or newer. Before adding it, verify your target deployment environments support these versions.
- SSLProtocol -all +TLSv1.2 + SSLProtocol -all +TLSv1.2 +TLSv1.3This is optional and depends on your minimum supported server versions across deployment targets.
|
|
|
|
|
|
Prepare
Description
Target issue
closes #issue-number-here
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Closes #12963,
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.