This is an automated email from the ASF dual-hosted git repository. chibenwa pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-jspf.git
commit a9e8bb79c1fd84454259d731d3425bf17bedd422 Author: Quan Tran <[email protected]> AuthorDate: Fri Oct 30 16:27:20 2026 +0700 [FIX] TrustedForwarderPolicy: add the trusted forwarder include getSPFRecord read the last directive at index size(), so it threw IndexOutOfBoundsException on every record: setUseTrustedForwarder(true) turned every SPF check into neutral. Once past that, the policy could not have worked either: - the Directive was built with a null qualifier, which Directive rejects with a PermErrorException the policy swallowed, so the include was never added; - its IncludeMechanism had neither the MacroExpand nor the SPFChecker the parser wires into an include; - the policies run on the included records too, so the include was added to them, down to the records of the trusted forwarder host and of the domains it includes, which included it again until permerror; - it changed the record it was given, which FallbackPolicy shares between checks: the includes piled up. Read the last directive, skip records without directives, give the include no qualifier and wire it with the MacroExpand and SPF of the check. The policy gets them as the terms do, through MacroExpandEnabled and SPFCheckEnabled, so its public constructor stays. Add the include to a copy of the record, and only to the record of the checked domain (or of its redirect=), not to the included ones: those are checked with ignoreExplanation set, as the exp= of an include target "MUST NOT be used" (RFC 7208 6.2). For this to hold in nested includes, an include now restores ignoreExplanation as it was, instead of clearing it while the outer include goes on. --- .../main/java/org/apache/james/jspf/impl/SPF.java | 35 ++++++++++- .../policies/local/TrustedForwarderPolicy.java | 48 +++++++++++++++- .../apache/james/jspf/terms/IncludeMechanism.java | 5 +- .../org/apache/james/jspf/SPFExecutorBaseTest.java | 67 ++++++++++++++++++++++ .../policies/local/TrustedForwarderPolicyTest.java | 50 ++++++++++++++++ 5 files changed, 200 insertions(+), 5 deletions(-) diff --git a/resolver/src/main/java/org/apache/james/jspf/impl/SPF.java b/resolver/src/main/java/org/apache/james/jspf/impl/SPF.java index 4c8f955..8b4f264 100644 --- a/resolver/src/main/java/org/apache/james/jspf/impl/SPF.java +++ b/resolver/src/main/java/org/apache/james/jspf/impl/SPF.java @@ -182,6 +182,36 @@ public class SPF implements SPFChecker { } + /** + * Applies the TrustedForwarderPolicy to the record of the checked domain, and of its redirect=, + * not to the included ones: the trusted forwarder host would be included again from its own + * record, and from the records it includes. + */ + private static final class TrustedForwarderPolicyChecker implements SPFChecker { + private final SPFChecker policyChecker; + + public TrustedForwarderPolicyChecker(TrustedForwarderPolicy policy) { + this.policyChecker = new SPFPolicyPostFilterChecker(policy); + } + + /** + * @see org.apache.james.jspf.core.SPFChecker#checkSPF(org.apache.james.jspf.core.SPFSession) + */ + public DNSLookupContinuation checkSPF(SPFSession spfData) + throws PermErrorException, TempErrorException, + NeutralException, NoneException { + // only an include ignores the explanation of the record it checks + if (spfData.ignoreExplanation()) { + return null; + } + return policyChecker.checkSPF(spfData); + } + + public String toString() { + return "TFC:" + policyChecker.toString(); + } + } + private DNSService dnsProbe; private SPFRecordParser parser; @@ -390,7 +420,10 @@ public class SPF implements SPFChecker { // trustedForwarder support is enabled if (useTrustedForwarder) { - policies.add(new SPFPolicyPostFilterChecker(new TrustedForwarderPolicy())); + TrustedForwarderPolicy trustedForwarderPolicy = new TrustedForwarderPolicy(); + trustedForwarderPolicy.enableMacroExpand(macroExpand); + trustedForwarderPolicy.enableSPFChecking(this); + policies.add(new TrustedForwarderPolicyChecker(trustedForwarderPolicy)); } policies.add(new SPFPolicyPostFilterChecker(new NeutralIfNotMatchPolicy())); diff --git a/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java b/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java index 2cca5d7..e1e968b 100644 --- a/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java +++ b/resolver/src/main/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicy.java @@ -19,7 +19,13 @@ package org.apache.james.jspf.policies.local; +import java.util.List; + +import org.apache.james.jspf.core.MacroExpand; +import org.apache.james.jspf.core.MacroExpandEnabled; import org.apache.james.jspf.core.SPF1Record; +import org.apache.james.jspf.core.SPFCheckEnabled; +import org.apache.james.jspf.core.SPFChecker; import org.apache.james.jspf.core.exceptions.NeutralException; import org.apache.james.jspf.core.exceptions.NoneException; import org.apache.james.jspf.core.exceptions.PermErrorException; @@ -35,7 +41,7 @@ import org.slf4j.LoggerFactory; * See http://www.trusted-forwarder.org for more informations * */ -public class TrustedForwarderPolicy implements PolicyPostFilter { +public class TrustedForwarderPolicy implements PolicyPostFilter, MacroExpandEnabled, SPFCheckEnabled { private static final Logger LOGGER = LoggerFactory.getLogger(TrustedForwarderPolicy.class); /** @@ -43,12 +49,40 @@ public class TrustedForwarderPolicy implements PolicyPostFilter { */ public static final String TRUSTED_FORWARDER_HOST = "spf.trusted-forwarder.org"; + /** + * The MacroExpand the include mechanism expands its host with + */ + private MacroExpand macroExpand; + + /** + * The SPFChecker the include mechanism checks the included domain with + */ + private SPFChecker spfChecker; + + /** + * @see org.apache.james.jspf.core.MacroExpandEnabled#enableMacroExpand(org.apache.james.jspf.core.MacroExpand) + */ + public void enableMacroExpand(MacroExpand macroExpand) { + this.macroExpand = macroExpand; + } + + /** + * @see org.apache.james.jspf.core.SPFCheckEnabled#enableSPFChecking(org.apache.james.jspf.core.SPFChecker) + */ + public void enableSPFChecking(SPFChecker checker) { + this.spfChecker = checker; + } + /** * @see org.apache.james.jspf.policies.PolicyPostFilter#getSPFRecord(java.lang.String, org.apache.james.jspf.core.SPF1Record) */ public SPF1Record getSPFRecord(String currentDomain, SPF1Record spfRecord) throws PermErrorException, TempErrorException, NoneException, NeutralException { if (spfRecord == null) return null; - String mechanism = ((Directive) spfRecord.getDirectives().get(spfRecord.getDirectives().size())).toString(); + List<Directive> directives = spfRecord.getDirectives(); + if (directives.isEmpty()) { + return spfRecord; + } + String mechanism = directives.get(directives.size() - 1).toString(); if (mechanism.equals("-all") || mechanism.equals("?all")) { LOGGER.debug("Add TrustedForwarderPolicy = include:{}", TRUSTED_FORWARDER_HOST); try { @@ -63,7 +97,15 @@ public class TrustedForwarderPolicy implements PolicyPostFilter { return this; } }.setHost(TRUSTED_FORWARDER_HOST); - spfRecord.getDirectives().add(spfRecord.getDirectives().size()-1, new Directive(null, trusted)); + trusted.enableMacroExpand(macroExpand); + trusted.enableSPFChecking(spfChecker); + // a copy: the record can be shared, as the cached ones of FallbackPolicy + SPF1Record filtered = new SPF1Record(spfRecord.getRecord()); + filtered.getDirectives().addAll(directives); + filtered.getModifiers().addAll(spfRecord.getModifiers()); + // no qualifier, as the parser gives an "include:" term + filtered.getDirectives().add(directives.size() - 1, new Directive("", trusted)); + return filtered; } catch (PermErrorException e) { // will never happen } diff --git a/resolver/src/main/java/org/apache/james/jspf/terms/IncludeMechanism.java b/resolver/src/main/java/org/apache/james/jspf/terms/IncludeMechanism.java index 1e7be8b..ddb4405 100644 --- a/resolver/src/main/java/org/apache/james/jspf/terms/IncludeMechanism.java +++ b/resolver/src/main/java/org/apache/james/jspf/terms/IncludeMechanism.java @@ -69,9 +69,11 @@ public class IncludeMechanism implements Mechanism, ConfigurationEnabled, SPFChe private String previousResult; private String previousResultExpanded; private String previousDomain; + private boolean previousIgnoreExplanation; private void restoreSession(SPFSession spfData) { - spfData.setIgnoreExplanation(false); + // true again when this include is nested in another one + spfData.setIgnoreExplanation(previousIgnoreExplanation); spfData.setCurrentDomain(previousDomain); spfData.setCurrentResult(previousResult); spfData.setCurrentResultExpanded(previousResultExpanded); @@ -134,6 +136,7 @@ public class IncludeMechanism implements Mechanism, ConfigurationEnabled, SPFChe this.previousResult = spfSession.getCurrentResult(); this.previousDomain = spfSession.getCurrentDomain(); this.previousResultExpanded = spfSession.getCurrentResultExpanded(); + this.previousIgnoreExplanation = spfSession.ignoreExplanation(); return this; } } diff --git a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java index 66186b7..1e7ac25 100644 --- a/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java +++ b/resolver/src/test/java/org/apache/james/jspf/SPFExecutorBaseTest.java @@ -7,6 +7,7 @@ import org.apache.james.jspf.executor.SPFResult; import org.apache.james.jspf.helpers.FakeResolver; import org.apache.james.jspf.impl.DNSServiceXBillImpl; import org.apache.james.jspf.impl.SPF; +import org.apache.james.jspf.policies.local.TrustedForwarderPolicy; import org.junit.Before; import org.junit.Test; import org.xbill.DNS.DClass; @@ -383,4 +384,70 @@ public abstract class SPFExecutorBaseTest { assertEquals(SPFErrorConstants.PERM_ERROR_CONV, result.getResult()); } + + @Test(timeout = 10000) + public void trustedForwarderShouldNotBeAddedToARedirectAfterANestedInclude() throws IOException { + String hostname = "trustedForwarderRedirectAfterInclude.spfexecutor.fake"; + String child = "child." + TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST; + String destination = "destination." + TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST; + FakeResolver fakeResolver = new FakeResolver(); + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, "v=spf1 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + "."), DClass.IN, 30L, + "v=spf1 include:" + child + " redirect=" + destination)); + fakeResolver.addRecord(new TXTRecord(Name.fromString(child + "."), DClass.IN, 30L, "v=spf1 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(destination + "."), DClass.IN, 30L, "v=spf1 ip4:192.0.2.202 -all")); + + SPF spf = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)); + spf.setUseTrustedForwarder(true); + + assertEquals(SPFErrorConstants.FAIL_CONV, spf.checkSPF("1.2.3.4", "a_user@" + hostname, hostname).getResult()); + assertEquals(SPFErrorConstants.PASS_CONV, spf.checkSPF("192.0.2.202", "a_user@" + hostname, hostname).getResult()); + } + + @Test(timeout = 10000) + public void trustedForwarderShouldKeepTheResultOfTheRecord() throws IOException { + String hostname = "trustedForwarderShouldKeepTheResultOfTheRecord.spfexecutor.fake"; + FakeResolver fakeResolver = new FakeResolver(); + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, "v=spf1 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + "."), DClass.IN, 30L, + "v=spf1 ip4:192.0.2.200 -all")); + + SPF spf = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)); + spf.setUseTrustedForwarder(true); + SPFResult result = spf.checkSPF("1.2.3.4", "a_user@" + hostname, hostname); + + assertEquals(SPFErrorConstants.FAIL_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void trustedForwarderShouldPassTheForwarderListedByTheTrustedForwarderHost() throws IOException { + String hostname = "trustedForwarderShouldPassListedForwarder.spfexecutor.fake"; + FakeResolver fakeResolver = new FakeResolver(); + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, "v=spf1 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + "."), DClass.IN, 30L, + "v=spf1 ip4:192.0.2.200 -all")); + + SPF spf = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)); + spf.setUseTrustedForwarder(true); + SPFResult result = spf.checkSPF("192.0.2.200", "a_user@" + hostname, hostname); + + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } + + @Test(timeout = 10000) + public void trustedForwarderShouldNotBeAddedToTheRecordsItIncludes() throws IOException { + String hostname = "trustedForwarderNotAddedToIncluded.spfexecutor.fake"; + String child = "child." + TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST; + FakeResolver fakeResolver = new FakeResolver(); + fakeResolver.addRecord(new TXTRecord(Name.fromString(hostname + "."), DClass.IN, 30L, "v=spf1 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + "."), DClass.IN, 30L, + "v=spf1 include:" + child + " ip4:192.0.2.201 -all")); + fakeResolver.addRecord(new TXTRecord(Name.fromString(child + "."), DClass.IN, 30L, "v=spf1 -all")); + + SPF spf = createCustomSPF(new DNSServiceXBillImpl(fakeResolver)); + spf.setUseTrustedForwarder(true); + SPFResult result = spf.checkSPF("192.0.2.201", "a_user@" + hostname, hostname); + + assertEquals(SPFErrorConstants.PASS_CONV, result.getResult()); + } } diff --git a/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java b/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java new file mode 100644 index 0000000..8b65a02 --- /dev/null +++ b/resolver/src/test/java/org/apache/james/jspf/policies/local/TrustedForwarderPolicyTest.java @@ -0,0 +1,50 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.jspf.policies.local; + +import static org.junit.Assert.assertEquals; + +import org.apache.james.jspf.core.SPF1Record; +import org.apache.james.jspf.impl.DefaultTermsFactory; +import org.apache.james.jspf.parser.RFC4408SPF1Parser; +import org.junit.Test; + +public class TrustedForwarderPolicyTest { + + @Test + public void getSPFRecordShouldAddTheIncludeBeforeTheLastAll() throws Exception { + SPF1Record record = new RFC4408SPF1Parser(new DefaultTermsFactory()).parse("v=spf1 ip4:192.0.2.1 -all"); + + SPF1Record filtered = new TrustedForwarderPolicy().getSPFRecord("example.com", record); + + assertEquals("[ip4:192.0.2.1, include:" + TrustedForwarderPolicy.TRUSTED_FORWARDER_HOST + ", -all]", + filtered.getDirectives().toString()); + } + + @Test + public void getSPFRecordShouldNotChangeTheGivenRecord() throws Exception { + // FallbackPolicy hands the same record to every check + SPF1Record record = new RFC4408SPF1Parser(new DefaultTermsFactory()).parse("v=spf1 -all"); + + new TrustedForwarderPolicy().getSPFRecord("example.com", record); + + assertEquals("[-all]", record.getDirectives().toString()); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
