XEP-0198 SASL2 / Bind2 integration work#3417
Conversation
# Conflicts: # xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java # xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
# Conflicts: # xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java
Previously, the -PLUS mechanism filter was only applied in getSASLMechanismsElement (advertisement), but not in getAvailableMechanismsForClientSession (eligibility check). This meant a client could request a -PLUS mechanism that was never advertised, and the eligibility check would pass — the rejection only happened deep inside the Java SASL implementation rather than as a clean protocol-level <invalid-mechanism/> failure. This commit moves the channel binding filter into getAvailableMechanismsForClientSession, making it the single source of truth for session-available mechanisms. getSASLMechanismsElement now simply iterates the result without additional filtering logic. Co-authored-by: Junie <junie@jetbrains.com>
…) checks
Replace all scattered mech.endsWith("-PLUS") checks with a single
requiresChannelBinding(String) helper method. This centralises the channel-binding
naming convention in one place so that any future change to how mechanisms signal
channel-binding support only needs to be made in one location.
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
…onManager.bindResource Co-authored-by: Junie <junie@jetbrains.com>
…(XEP-0386) XEP-0198 was previously supported in isolation but had no integration with the SASL2 / Bind2 inline-feature negotiation that this branch also supports. This commit wires the two together at all three integration points defined by the specifications. 1. Inline feature advertisement (XEP-0388 §6.3.1) SASLAuthentication.getSASLMechanismsElement() now adds an <sm/> element (urn:xmpp:sm:3) inside the <inline/> child of the SASL2 <authentication/> feature, but only when stream management is globally active. 2. SM <enable> inside Bind2 (XEP-0386) A new Bind2InlineHandler implementation, Bind2StreamManagementHandler, handles <enable xmlns='urn:xmpp:sm:3'/> elements that arrive inside the Bind2 <bind/> element. It calls the new StreamManager.enableAndBuildElement() method (which performs the same work as the existing private enable() but returns the <enabled/> element instead of sending it) and adds the result to the <bound/> element in the SASL2 <success/> stanza, so the client receives everything in a single round-trip. The handler is registered in SessionManager.start() and unregistered in SessionManager.stop(). 3. SM <resume> inside SASL2 <authenticate> (XEP-0388 §6.3.2) When a client includes a <resume xmlns='urn:xmpp:sm:3'/> element inside its SASL2 <authenticate/> stanza, SASLAuthentication.handle() stores it on the session. After SASL authentication succeeds, authenticationSuccessful() detects the stored element and calls the new StreamManager.processSasl2Resume() method. That method mirrors the existing startResume() logic but calls the new LocalSession.reattachForSasl2() instead of reattach(): the new variant takes over the connection and builds the <resumed/> element without sending it, so the caller can embed it inside the SASL2 <success/> stanza. Supporting refactors - StreamManager.onResume() is decomposed into buildResumedElement(), processClientAcknowledgementPublic(), and redeliverUnackedStanzas() so the SASL2 resume path can reuse the same logic without duplicating it. - StreamManager.enable() is decomposed into enableInternal() (returns the element) and the original enable() (sends it), with the new public enableAndBuildElement() delegating to enableInternal(). - LocalSession gains reattachForSasl2() alongside the existing reattach(). Tests - StreamManagerTest: three new tests for StreamManager.featureElement(). - Bind2StreamManagementHandlerTest: seven tests covering enable/resume attribute parsing, failure handling, and rejection of unexpected elements. - SASLAuthenticationTest: three new tests verifying that the <sm/> inline feature is present in SASL2 advertisements when SM is active, absent when SM is inactive, and absent from SASL1 advertisements entirely. Co-authored-by: Junie <junie@jetbrains.com>
When a session is resumed inline via SASL2 (XEP-0388 + XEP-0198), the
server must send pending unacknowledged stanzas only *after* the stream
features that follow the <success/> element, not before.
Previously, reattachForSasl2() called redeliverUnackedStanzas() directly,
which meant stanzas were sent before <success/> was even delivered to the
client, let alone the post-success stream features.
Fix:
- Add a boolean flag pendingSasl2Redelivery to StreamManager, with
setPendingSasl2Redelivery(boolean) and redeliverIfPendingSasl2(JID).
- reattachForSasl2() in LocalSession now sets the flag instead of
calling redeliverUnackedStanzas() directly.
- StanzaHandler calls redeliverIfPendingSasl2() immediately after
delivering stream features following a successful SASL2 authenticate
or response, ensuring the correct ordering:
1. <success/> (with embedded <resumed/>)
2. stream features
3. unacknowledged stanzas redelivered
Three new unit tests in StreamManagerTest verify the flag semantics.
Co-authored-by: Junie <junie@jetbrains.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
.github/workflows/continuous-integration-workflow.yml (1)
332-382: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winJob runs with default (broad)
GITHUB_TOKENpermissions.Static analysis flags this job as lacking an explicit
permissions:block. Since this job only needs to checkout, download artifacts, and upload artifacts, consider scoping permissions (e.g.,contents: read) to follow least privilege.🔒 Suggested fix
conversations: name: Execute Conversations e2e tests (${{ matrix.name }}) runs-on: ubuntu-latest needs: build + permissions: + contents: read strategy:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/continuous-integration-workflow.yml around lines 332 - 382, The conversations e2e job is missing an explicit permissions scope, so it inherits the broad default GITHUB_TOKEN permissions. Add a job-level permissions block on the conversations workflow job and restrict it to the minimum needed for its steps (checkout, artifact download, and artifact upload), such as read-only repository contents. Update the conversations job definition in the workflow alongside the existing matrix and steps so the permissions are clearly scoped.Source: Linters/SAST tools
xmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.java (1)
450-587: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
processSasl2Resumelargely duplicatesstartResume; consider extracting shared validation.Lines 464–586 replicate almost all of
startResume(...)(h/previd parsing,allowResume()/isAuthenticated()/authToken gating, previd decode, route lookup, cluster/location handling, SM-compatibility and ack validation). The only real deltas are error-emission style (<failed/>vs closing the stream) andreattachvsreattachForSasl2. Duplicated security/validation logic tends to diverge over time, which is risky here. Extracting the common validation into a shared helper (returning the resolvedotherSession) would keep the two entry points in lockstep.Note also that, as in
startResume,allowResume()returnsfalsefor an anonymousAuthToken, so the anonymous branch at Lines 518–520 is unreachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.java` around lines 450 - 587, processSasl2Resume duplicates nearly all of startResume’s validation and session lookup logic, so extract the shared resume-validation flow into a helper that resolves and returns the target LocalClientSession. Reuse the common checks in StreamManager for h/previd parsing, allowResume(), isAuthenticated(), authToken retrieval, previd decoding, route lookup, namespace/ack validation, and detached-session handling, while keeping only the SASL2-specific differences in processSasl2Resume (failed stanza emission and reattachForSasl2). Also remove the unreachable anonymous-auth branch since allowResume() already rejects anonymous AuthToken cases.xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java (1)
212-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the duplicated post-auth SASL2 feature/redelivery block.
Lines 212-216 and 224-228 are identical. Consider a small private helper (e.g.
deliverSasl2StreamFeatures()) to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java` around lines 212 - 228, The SASL2 post-authentication feature delivery and stream-manager redelivery logic is duplicated in StanzaHandler’s SASL handling branch. Extract the repeated block into a small private helper such as deliverSasl2StreamFeatures() and call it from both authenticated paths, so the generateFeatures(), session.deliverRawText(), and session.getStreamManager().redeliverIfPendingSasl2() behavior stays in one place.xmppserver/src/main/java/org/jivesoftware/openfire/handler/IQMessageCarbonsHandler.java (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the
NAMESPACEconstant instead of a duplicated string literal.
stop()hardcodes"urn:xmpp:carbons:2"instead of reusing theNAMESPACEfield already declared in this class (Line 38), risking silent drift if the constant ever changes.♻️ Proposed fix
`@Override` public void stop() { super.stop(); - Bind2Request.unregisterElementHandler("urn:xmpp:carbons:2"); + Bind2Request.unregisterElementHandler(NAMESPACE); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/handler/IQMessageCarbonsHandler.java` around lines 89 - 93, The stop() method in IQMessageCarbonsHandler should not hardcode the carbons namespace string; replace the duplicated "urn:xmpp:carbons:2" literal with the existing NAMESPACE constant. Update the Bind2Request.unregisterElementHandler call in stop() to use NAMESPACE so the handler registration and teardown stay in sync if the namespace changes.xmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2StreamManagementHandler.java (1)
70-71: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
resume="yes"is still accepted here The inline and non-inline SM paths both acceptyes, so there’s no divergence. For strict XEP-0198 boolean parsing, narrow both handlers totrue/false/1/0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2StreamManagementHandler.java` around lines 70 - 71, The Stream Management resume flag parsing in Bind2StreamManagementHandler still accepts resume="yes", which should be narrowed to strict XEP-0198 boolean handling. Update the parsing logic in the resume attribute handling so it only treats true/false and 1/0 as valid values, and make the same change in the matching non-inline SM path to keep both handlers consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2CarbonsHandler.java`:
- Around line 14-17: The Bind2CarbonsHandler.handleElement logic currently
treats any element name other than "enable" as a disable request and always
succeeds, which lets malformed input slip through. Update handleElement to
explicitly validate the incoming Element name against the allowed carbons tags
(using the same rule set as IQMessageCarbonsHandler.handleIQ), only toggling
session.setMessageCarbonsEnabled for recognized values and rejecting unknown
names with a bad_request-style failure instead of returning success.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java`:
- Around line 205-212: Normalize or validate `clientTag` in
`Bind2Request.generateResourceString` before appending it to the resource, since
it is taken directly from `<tag>` and later used in `new JID(username,
serverName, resource, true)` without stringprep. Update the `clientTag` handling
path so only a compliant resourcepart is concatenated, either by rejecting
invalid values or applying the appropriate prep logic before the `StringBuilder`
append.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java`:
- Around line 212-216: The SASL2 authenticated branch in StanzaHandler.handle()
is sending <stream:features/> immediately, which can race ahead of the Bind2
<success/> path. Move the feature delivery and
StreamManager.redeliverIfPendingSasl2 call out of this synchronous block and
into the SASL2 Bind2 whenComplete(...) completion path (or gate it until Bind2
finishes) so the features stanza cannot overtake success for inline Bind2
sessions.
- Line 217: The SASL branch in StanzaHandler’s stanza processing is missing
proper gating for abort, so a bare abort can be handled outside an active SASL
exchange. Update the condition that checks startedSASL and the tag so both
"response" and "abort" are covered by the startedSASL check, keeping abort from
being processed unless SASL has already started.
---
Nitpick comments:
In @.github/workflows/continuous-integration-workflow.yml:
- Around line 332-382: The conversations e2e job is missing an explicit
permissions scope, so it inherits the broad default GITHUB_TOKEN permissions.
Add a job-level permissions block on the conversations workflow job and restrict
it to the minimum needed for its steps (checkout, artifact download, and
artifact upload), such as read-only repository contents. Update the
conversations job definition in the workflow alongside the existing matrix and
steps so the permissions are clearly scoped.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2StreamManagementHandler.java`:
- Around line 70-71: The Stream Management resume flag parsing in
Bind2StreamManagementHandler still accepts resume="yes", which should be
narrowed to strict XEP-0198 boolean handling. Update the parsing logic in the
resume attribute handling so it only treats true/false and 1/0 as valid values,
and make the same change in the matching non-inline SM path to keep both
handlers consistent.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/handler/IQMessageCarbonsHandler.java`:
- Around line 89-93: The stop() method in IQMessageCarbonsHandler should not
hardcode the carbons namespace string; replace the duplicated
"urn:xmpp:carbons:2" literal with the existing NAMESPACE constant. Update the
Bind2Request.unregisterElementHandler call in stop() to use NAMESPACE so the
handler registration and teardown stay in sync if the namespace changes.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java`:
- Around line 212-228: The SASL2 post-authentication feature delivery and
stream-manager redelivery logic is duplicated in StanzaHandler’s SASL handling
branch. Extract the repeated block into a small private helper such as
deliverSasl2StreamFeatures() and call it from both authenticated paths, so the
generateFeatures(), session.deliverRawText(), and
session.getStreamManager().redeliverIfPendingSasl2() behavior stays in one
place.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.java`:
- Around line 450-587: processSasl2Resume duplicates nearly all of startResume’s
validation and session lookup logic, so extract the shared resume-validation
flow into a helper that resolves and returns the target LocalClientSession.
Reuse the common checks in StreamManager for h/previd parsing, allowResume(),
isAuthenticated(), authToken retrieval, previd decoding, route lookup,
namespace/ack validation, and detached-session handling, while keeping only the
SASL2-specific differences in processSasl2Resume (failed stanza emission and
reattachForSasl2). Also remove the unreachable anonymous-auth branch since
allowResume() already rejects anonymous AuthToken cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0e29e89-9e16-4a8a-bd98-e21454134564
📒 Files selected for processing (34)
.github/actions/conversationstest-action/action.yml.github/workflows/continuous-integration-workflow.ymlbuild/ci/conversations/configs/sasl2.xmlbuild/ci/conversations/flows/sasl2.yamldocumentation/openfire.doapi18n/src/main/resources/openfire_i18n.propertiesxmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/SessionPacketRouter.javaxmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.javaxmppserver/src/main/java/org/jivesoftware/openfire/csi/CsiModule.javaxmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2CarbonsHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2StreamManagementHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/handler/IQMessageCarbonsHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/http/HttpSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2InlineHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/UserAgentInfo.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalClientSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalIncomingServerSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/websocket/WebSocketClientStanzaHandler.javaxmppserver/src/test/java/org/jivesoftware/openfire/handler/Bind2StreamManagementHandlerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/Bind2InlineHandlerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/Bind2RequestProcessingTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/Bind2RequestTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/SASLAuthenticationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/UserAgentInfoTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/TestSaslMechanism.javaxmppserver/src/test/java/org/jivesoftware/openfire/streammanagement/StreamManagerTest.java
| public boolean handleElement(LocalClientSession session, Element bound, Element element) { | ||
| session.setMessageCarbonsEnabled(element.getName().equals("enable")); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate element name instead of treating anything non-"enable" as "disable".
Unlike IQMessageCarbonsHandler.handleIQ, which rejects unrecognized element names with bad_request, this inline handler silently maps any element other than <enable/> to "disable" and always reports success. Malformed/unexpected client input goes unnoticed.
🛡️ Proposed fix
public boolean handleElement(LocalClientSession session, Element bound, Element element) {
- session.setMessageCarbonsEnabled(element.getName().equals("enable"));
- return true;
+ final String name = element.getName();
+ if (!"enable".equals(name) && !"disable".equals(name)) {
+ return false;
+ }
+ session.setMessageCarbonsEnabled("enable".equals(name));
+ return true;
}📝 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.
| public boolean handleElement(LocalClientSession session, Element bound, Element element) { | |
| session.setMessageCarbonsEnabled(element.getName().equals("enable")); | |
| return true; | |
| } | |
| public boolean handleElement(LocalClientSession session, Element bound, Element element) { | |
| final String name = element.getName(); | |
| if (!"enable".equals(name) && !"disable".equals(name)) { | |
| return false; | |
| } | |
| session.setMessageCarbonsEnabled("enable".equals(name)); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/handler/Bind2CarbonsHandler.java`
around lines 14 - 17, The Bind2CarbonsHandler.handleElement logic currently
treats any element name other than "enable" as a disable request and always
succeeds, which lets malformed input slip through. Update handleElement to
explicitly validate the incoming Element name against the allowed carbons tags
(using the same rule set as IQMessageCarbonsHandler.handleIQ), only toggling
session.setMessageCarbonsEnabled for recognized values and rejecting unknown
names with a bad_request-style failure instead of returning success.
| public String generateResourceString(UserAgentInfo userAgentInfo) { | ||
| StringBuilder resource = new StringBuilder(); | ||
|
|
||
| // Add the client tag if available | ||
| if (clientTag != null && !clientTag.isEmpty()) { | ||
| resource.append(clientTag); | ||
| resource.append('/'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where generateResourceString is consumed and how the resource becomes a JID.
rg -nP -C4 'generateResourceString' --type=java
# Inspect resource/JID construction for prep/validation.
rg -nP -C3 '(new JID\(|prepResource|resourceprep|Resourceprep)' --type=java xmppserver/src/main/java/org/jivesoftware/openfire/netRepository: igniterealtime/Openfire
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map Bind2Request and inspect the relevant method.
ast-grep outline xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java --view expanded || true
sed -n '1,320p' xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java
echo '--- usages of generateResourceString ---'
rg -n --hidden --glob '*.java' 'generateResourceString\s*\(' .
echo '--- resource/JID prep and validation references ---'
rg -n --hidden --glob '*.java' '(resourceprep|prepResource|validateResource|resourcepart|new JID\(|JID\()' xmppserver/src/main/java .Repository: igniterealtime/Openfire
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SASLAuthentication around generateResourceString usage ---'
sed -n '740,805p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
echo '--- JID implementation outline ---'
ast-grep outline xmppserver/src/main/java/org/jivesoftware/openfire/JID.java --view expanded || true
sed -n '1,260p' xmppserver/src/main/java/org/jivesoftware/openfire/JID.java
echo '--- resource-related methods in JID ---'
rg -n --hidden --glob '*.java' 'resourceprep|prepResource|setResource|getResource|resourcePart|resource part|RESOURCE' xmppserver/src/main/java/org/jivesoftware/openfire/JID.java xmppserver/src/main/java/org/jivesoftware/utilRepository: igniterealtime/Openfire
Length of output: 4655
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate JID.java ---'
fd -a 'JID.java' .
echo '--- locate bindResource and related validation ---'
rg -n --hidden --glob '*.java' 'bindResource\(|createJID\(|new JID\(' xmppserver src .
echo '--- inspect SessionManager.bindResource call site and implementation ---'
rg -n --hidden --glob '*.java' 'class SessionManager|bindResource\(' xmppserver src .Repository: igniterealtime/Openfire
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate JID implementation ---'
fd -a 'JID.java' xmppserver
echo '--- locate SessionManager implementation ---'
fd -a 'SessionManager.java' xmppserver
echo '--- locate bindResource definition and nearby calls ---'
rg -n -C 4 'bindResource\(' xmppserver/src/main/java --glob '*.java'Repository: igniterealtime/Openfire
Length of output: 5623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- all JID.java files in repo ---'
git ls-files '**/JID.java' '**/*JID.java'
echo '--- SessionManager.bindResource implementation ---'
sed -n '1475,1565p' xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
echo '--- IQBindHandler resource handling ---'
sed -n '80,120p' xmppserver/src/main/java/org/jivesoftware/openfire/handler/IQBindHandler.java
echo '--- tests around invalid resource/JID behavior ---'
sed -n '1,140p' xmppserver/src/test/java/org/jivesoftware/util/JIDTest.javaRepository: igniterealtime/Openfire
Length of output: 10996
🌐 Web query:
org.xmpp.packet.JID constructor resource validation resourceprep Openfire
💡 Result:
In Openfire, the org.xmpp.packet.JID class handles the validation and normalization of XMPP address components, including the resource [1][2]. The JID constructors and associated methods ensure that XMPP compliance is maintained through stringprep profiles [1][2]. Key details regarding resource validation and resourceprep include: Normalization: The JID class uses resourceprep to normalize the resource portion of a JID [1][2]. This process is case-sensitive, which is a requirement for XMPP address comparison [1][3][2]. Constructors: The standard JID(String node, String domain, String resource) constructor performs validation and applies stringprep operations to the components [1][2]. An overloaded constructor, JID(String node, String domain, String resource, boolean skipStringprep), allows developers to bypass these operations if they are certain the input is already compliant or if performance optimization is required [1][2]. Passing true for the skipStringprep parameter disables the automatic application of resourceprep and other stringprep profiles [1][2]. Validation Errors: If the provided components, including the resource, cannot be processed or validated (e.g., due to invalid characters or formatting), the constructor throws an IllegalArgumentException [1][4][2]. Direct Usage: Beyond the JID constructor, Openfire exposes a static method JID.resourceprep(String resource) that can be called directly to validate or normalize a resource string independently of JID construction [2][5][6]. This method may throw a StringprepException if the resource does not conform to the expected requirements [2][5][6]. These mechanisms ensure that JIDs used within Openfire are well-formed and that resource-specific comparisons function according to XMPP standards [1][2].
Citations:
- 1: https://download.igniterealtime.org/openfire/docs/3.3.0/documentation/javadoc/org/xmpp/packet/JID.html
- 2: https://download.igniterealtime.org/openfire/docs/3.6.3/documentation/javadoc/org/xmpp/packet/JID.html
- 3: https://download.igniterealtime.org/openfire/docs/3.4.4/documentation/javadoc/org/xmpp/packet/JID.html
- 4: http://download.igniterealtime.org/wildfire/docs/latest/documentation/javadoc/org/xmpp/packet/JID.html
- 5: https://download.igniterealtime.org/openfire/docs/3.5.0/documentation/javadoc/org/xmpp/packet/JID.html
- 6: https://download.igniterealtime.org/openfire/docs/3.6.2/documentation/javadoc/org/xmpp/packet/JID.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' xmppserver/src/test/java/org/jivesoftware/openfire/net/Bind2RequestTest.javaRepository: igniterealtime/Openfire
Length of output: 5451
Normalize clientTag before appending it to the resource
clientTag is copied verbatim from <tag>, and the SASL2 bind path later calls new JID(username, serverName, resource, true), which skips stringprep. Reject or prep the tag before concatenation so the final resourcepart stays compliant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java`
around lines 205 - 212, Normalize or validate `clientTag` in
`Bind2Request.generateResourceString` before appending it to the resource, since
it is taken directly from `<tag>` and later used in `new JID(username,
serverName, resource, true)` without stringprep. Update the `clientTag` handling
path so only a compliant resourcepart is concatenated, either by rejecting
invalid values or applying the appropriate prep logic before the `StringBuilder`
append.
| if (saslStatus == SASLAuthentication.Status.authenticated && usingSASL2) { | ||
| Element features = generateFeatures(); | ||
| session.deliverRawText(features.asXML()); | ||
| session.getStreamManager().redeliverIfPendingSasl2(new JID(null, session.getServerName(), null, true)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect bindResource to determine whether it can complete asynchronously.
fd 'SessionManager.java' -p 'main/java' | xargs -r ast-grep run \
--pattern 'public $_ bindResource($$$) { $$$ }' --lang javaRepository: igniterealtime/Openfire
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and map their structure.
git ls-files | rg '(^|/)(StanzaHandler\.java|SessionManager\.java|SASLAuthentication\.java|StreamManager\.java)$'Repository: igniterealtime/Openfire
Length of output: 472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant implementation slices with line numbers.
for f in \
xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java \
xmppserver/src/main/java/org/jivesoftware/openfire/spi/SessionManager.java \
xmppserver/src/main/java/org/jivesoftware/openfire/sasl/SASLAuthentication.java \
xmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.java
do
echo "===== $f ====="
wc -l "$f"
doneRepository: igniterealtime/Openfire
Length of output: 518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant code around the reported lines.
sed -n '190,235p' xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java
printf '\n===== SASLAuthentication =====\n'
sed -n '730,820p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
printf '\n===== SessionManager =====\n'
sed -n '1,260p' xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
printf '\n===== StreamManager =====\n'
sed -n '1,260p' xmppserver/src/main/java/org/jivesoftware/openfire/streammanagement/StreamManager.javaRepository: igniterealtime/Openfire
Length of output: 31342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect bindResource plus the delivery path it uses.
rg -n "bindResource\\(" xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java
printf '\n===== bindResource implementation =====\n'
sed -n '1320,1455p' xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
printf '\n===== deliverRawText implementations =====\n'
rg -n "deliverRawText\\(" xmppserver/src/main/java/org/jivesoftware/openfire -g '*.java'Repository: igniterealtime/Openfire
Length of output: 20891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect bindResource implementation and any async completion paths.
sed -n '1475,1615p' xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
printf '\n===== CompletableFuture usages in SessionManager =====\n'
rg -n "CompletableFuture|supplyAsync|completeAsync|whenComplete|thenCompose|thenApply|Executor" xmppserver/src/main/java/org/jivesoftware/openfire/SessionManager.java
printf '\n===== LocalSession deliverRawText implementation =====\n'
sed -n '450,510p' xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.java
printf '\n===== NettyConnection deliverRawText implementation =====\n'
sed -n '330,390p' xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.javaRepository: igniterealtime/Openfire
Length of output: 16631
Defer <stream:features/> until the SASL2 Bind2 whenComplete(...) path finishes. bindResource(...) runs on bindConflictExecutor, so the <success/> stanza can arrive after handle() already returns here. That lets <stream:features/> overtake <success/> for inline Bind2 sessions; move the feature/redelivery send into the completion stage or suppress it until Bind2 completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java`
around lines 212 - 216, The SASL2 authenticated branch in StanzaHandler.handle()
is sending <stream:features/> immediately, which can race ahead of the Bind2
<success/> path. Move the feature delivery and
StreamManager.redeliverIfPendingSasl2 call out of this synchronous block and
into the SASL2 Bind2 whenComplete(...) completion path (or gate it until Bind2
finishes) so the features stanza cannot overtake success for inline Bind2
sessions.
| session.deliverRawText(features.asXML()); | ||
| session.getStreamManager().redeliverIfPendingSasl2(new JID(null, session.getServerName(), null, true)); | ||
| } | ||
| } else if (startedSASL && "response".equals(tag) || "abort".equals(tag)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '180,250p' xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java
rg -n "handle\(.*abort|class SASLAuthentication|ABORTED|startedSASL" xmppserver/src/main/java -SRepository: igniterealtime/Openfire
Length of output: 4811
🏁 Script executed:
sed -n '180,250p' xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java
rg -n "handle\\(.*abort|class SASLAuthentication|ABORTED|startedSASL" xmppserver/src/main/java -SRepository: igniterealtime/Openfire
Length of output: 4811
🏁 Script executed:
sed -n '430,530p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
sed -n '200,250p' xmppserver/src/main/java/org/jivesoftware/openfire/net/RespondingServerStanzaHandler.javaRepository: igniterealtime/Openfire
Length of output: 9577
🏁 Script executed:
rg -n 'startedSASL && "response"\.equals\(tag\) \|\| "abort"\.equals\(tag\)|startedSASL && \("response"\.equals\(tag\) \|\| "abort"\.equals\(tag\)\)|"abort"\.equals\(tag\)' xmppserver/src/main/java -S
sed -n '120,160p' xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.javaRepository: igniterealtime/Openfire
Length of output: 1808
🏁 Script executed:
rg -n "catch \\(SaslFailureException|Status\\.failed|retry|increment.*retry|ABORTED|send.*failure" xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java -n -S
sed -n '530,700p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaRepository: igniterealtime/Openfire
Length of output: 9886
🏁 Script executed:
rg -n "authenticationFailed\\(|recordFailedLogin|retry" xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java -S
sed -n '700,820p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaRepository: igniterealtime/Openfire
Length of output: 8350
🏁 Script executed:
sed -n '824,900p' xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaRepository: igniterealtime/Openfire
Length of output: 3734
abort should be gated by startedSASL as well
&& binds tighter than ||, so this parses as (startedSASL && "response".equals(tag)) || "abort".equals(tag). As written, a bare <abort/> will enter the SASL handler even when no SASL exchange is in progress, incrementing auth retries and potentially closing the connection. Group both tags under the startedSASL check.
Proposed fix
- } else if (startedSASL && "response".equals(tag) || "abort".equals(tag)) {
+ } else if (startedSASL && ("response".equals(tag) || "abort".equals(tag))) {📝 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.
| } else if (startedSASL && "response".equals(tag) || "abort".equals(tag)) { | |
| } else if (startedSASL && ("response".equals(tag) || "abort".equals(tag))) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java` at
line 217, The SASL branch in StanzaHandler’s stanza processing is missing proper
gating for abort, so a bare abort can be handled outside an active SASL
exchange. Update the condition that checks startedSASL and the tag so both
"response" and "abort" are covered by the startedSASL check, keeping abort from
being processed unless SASL has already started.
2 participants
Built on top of both SASL2 and Bind2 PRs, only the last two commits are relevent.