Skip to content

Commit 91b4c6c

Browse files
netmackannetmackan
committed
Merge branch 'fb-DSS-3249-Regression-PDFSigner-fails-signing-if-custom-image-path-is-used' into 'main'
DSS-3249: Changed to the intended constructor of PDFSigner so the allowlist isn't treated as null. See merge request signserver/signserver!565
2 parents 4c31478 + 1915041 commit 91b4c6c

File tree

4 files changed

+53
-33
lines changed

4 files changed

+53
-33
lines changed

‎signserver/modules/SignServer-Module-PDFSigner/src/main/java/org/signserver/module/pdfsigner/PDFSigner.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -425,7 +425,7 @@ public Response processData(Request signRequest,
425425
// retrieve and preprocess configuration parameter values
426426
// XXX: Note: below a new instance of configError is passed in. Actually we do not want to collect errors at this stage but will do it like this for now
427427
final ArrayList<String> processingErrors = new ArrayList<>();
428-
final PDFSignerParameters params = new PDFSignerParameters(workerId, config, processingErrors, metadata, allowPropertyOverride);
428+
final PDFSignerParameters params = new PDFSignerParameters(workerId, config, processingErrors, metadata, allowPropertyOverride, allowedImagePaths);
429429
if (!processingErrors.isEmpty()) {
430430
throw new IllegalRequestException(processingErrors.toString());
431431
}

‎signserver/modules/SignServer-Module-PDFSigner/src/main/java/org/signserver/module/pdfsigner/PDFSignerParameters.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,6 @@ public class PDFSignerParameters {
112112

113113
private final Set<String> allowPropertyOverride;
114114

115-
public PDFSignerParameters(int workerId, WorkerConfig config, final List<String> configErrors, Map<String, String> requestMetadata, final Set<String> allowPropertyOverride) throws IllegalRequestException, SignServerException {
116-
this(workerId, config, configErrors, requestMetadata, allowPropertyOverride, null);
117-
}
118-
119115
public PDFSignerParameters(int workerId, WorkerConfig config, final List<String> configErrors, Map<String, String> requestMetadata, final Set<String> allowPropertyOverride, Set<Path> allowListedImagePaths) throws IllegalRequestException, SignServerException {
120116
this.workerId = workerId;
121117
this.config = config;

‎signserver/modules/SignServer-Module-PDFSigner/src/test/java/org/signserver/module/pdfsigner/PDFSignerParametersUnitTest.java‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import java.util.HashSet;
1717
import java.util.LinkedList;
1818
import java.util.Set;
19-
import org.apache.commons.codec.binary.Base64;
19+
import java.util.Collections;
2020
import org.apache.log4j.Logger;
2121
import org.junit.Test;
2222
import static org.junit.Assert.*;
@@ -52,7 +52,7 @@ public void testOverrideAddVisibleSignature() throws Exception {
5252
final LinkedList<String> configErrors = new LinkedList<>();
5353

5454
// when
55-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
55+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
5656

5757
// then
5858
assertTrue("ADD_VISIBLE_SIGNATURE overridden",
@@ -77,7 +77,7 @@ public void testOverrideVisible_sig_page() throws Exception {
7777
final LinkedList<String> configErrors = new LinkedList<>();
7878

7979
// when
80-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
80+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
8181

8282
// then
8383
assertEquals("VISIBLE_SIGNATURE_PAGE overridden", "4", instance.getVisible_sig_page());
@@ -101,7 +101,7 @@ public void testOverrideVisible_sig_rectangle() throws Exception {
101101
final LinkedList<String> configErrors = new LinkedList<>();
102102

103103
// when
104-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
104+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
105105

106106
// then
107107
assertEquals("VISIBLE_SIGNATURE_RECTANGLE overridden", "7,8,9,10", instance.getVisible_sig_rectangle());
@@ -130,7 +130,7 @@ public void testOverrideUseTimestampTrue() throws Exception {
130130
final LinkedList<String> configErrors = new LinkedList<>();
131131

132132
// when
133-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
133+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
134134

135135
// then
136136
assertFalse("USE_TIMESTAMP overridden", instance.isUseTimestamp());
@@ -155,7 +155,7 @@ public void testOverrideUseTimestampFalse() throws Exception {
155155
final LinkedList<String> configErrors = new LinkedList<>();
156156

157157
// when
158-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
158+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
159159

160160
// then
161161
assertTrue("USE_TIMESTAMP overridden", instance.isUseTimestamp());
@@ -179,7 +179,7 @@ public void testOverrideEmbedCrl() throws Exception {
179179
final LinkedList<String> configErrors = new LinkedList<>();
180180

181181
// when
182-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
182+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
183183

184184
// then
185185
assertTrue("EMBED_CRL overridden", instance.isEmbed_crl());
@@ -203,7 +203,7 @@ public void testOverrideEmbedOcspResponse() throws Exception {
203203
final LinkedList<String> configErrors = new LinkedList<>();
204204

205205
// when
206-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
206+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
207207

208208
// then
209209
assertTrue("EMBED_OCSP_RESPONSE overridden", instance.isEmbed_ocsp_response());
@@ -226,7 +226,7 @@ public void testOverrideRemovePermissions() throws Exception {
226226
final LinkedList<String> configErrors = new LinkedList<>();
227227

228228
// when
229-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
229+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
230230

231231
// then
232232
assertEquals("REMOVE_PERMISSIONS overridden",
@@ -251,7 +251,7 @@ public void testOverrideSetPermissions() throws Exception {
251251
final LinkedList<String> configErrors = new LinkedList<>();
252252

253253
// when
254-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
254+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
255255

256256
// then
257257
assertEquals("SET_PERMISSIONS overridden",
@@ -276,7 +276,7 @@ public void testOverrideSetOwnerPassword() throws Exception {
276276
final LinkedList<String> configErrors = new LinkedList<>();
277277

278278
// when
279-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
279+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
280280

281281
// then
282282
assertEquals("REMOVE_PERMISSIONS overridden",
@@ -303,7 +303,7 @@ public void testOverrideVisibleSignatureCustomImageBase64() throws Exception {
303303
final LinkedList<String> configErrors = new LinkedList<>();
304304

305305
// when
306-
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride);
306+
PDFSignerParameters instance = new PDFSignerParameters(101, config, configErrors, requestMetadata, allowPropertyOverride, Collections.emptySet());
307307

308308
// then
309309
assertEquals("VISIBLE_SIGNATURE_CUSTOM_IMAGE_BASE64 overridden", image, instance.getVisible_sig_custom_image_base64());

‎signserver/modules/SignServer-Module-PDFSigner/src/test/java/org/signserver/module/pdfsigner/PDFSignerUnitTest.java‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,7 +1734,7 @@ public void test23Illegal_Value_Gives_Configuration_Errors() {
17341734
workerConfig.setProperty("ARCHIVETODISK", "TRUE");
17351735
workerConfig.setProperty("ARCHIVETODISK_PATH_BASE", "/this/path/is/not/allowed");
17361736

1737-
final MockedPathingPDFSigner instance = new MockedPathingPDFSigner();
1737+
final MockedPathingPDFSigner instance = new MockedPathingPDFSigner(null);
17381738

17391739
instance.init(WORKER1, workerConfig, null, null);
17401740

@@ -2374,6 +2374,35 @@ public void testAllowSigningWithoutOwnerPassword() throws Exception {
23742374
workerSession.reloadConfiguration(WORKER1);
23752375
}
23762376

2377+
/**
2378+
* Test signing with visible signature from an allowed file path.
2379+
* @throws Exception in case of error
2380+
*/
2381+
@Test
2382+
public void testSignWithAllowedVisibleSignaturePath() throws Exception {
2383+
2384+
workerSession.setWorkerProperty(WORKER1, "ADD_VISIBLE_SIGNATURE", "true");
2385+
workerSession.setWorkerProperty(WORKER1, "VISIBLE_SIGNATURE_CUSTOM_IMAGE_PATH", ALLOWED_RELATIVE_PATH + "/primekey.png");
2386+
workerSession.reloadConfiguration(WORKER1);
2387+
2388+
ReadableData requestData = createRequestDataKeepingFile(sampleOk);
2389+
try (CloseableWritableData responseData = createResponseData(true)) {
2390+
2391+
final SignatureRequest request = new SignatureRequest(100,
2392+
requestData, responseData);
2393+
2394+
final SignatureResponse response = (SignatureResponse)
2395+
processSession.process(createAdminInfo(), new WorkerIdentifier(WORKER1), request, new RequestContext(true));
2396+
assertEquals("requestId", 100, response.getRequestID());
2397+
2398+
Certificate signercert = response.getSignerCertificate();
2399+
assertNotNull(signercert);
2400+
2401+
assertTrue("data processed", responseData.toReadableData().getLength() > 0);
2402+
assertTrue("data processed", responseData.toReadableData().getAsByteArray().length > 0);
2403+
}
2404+
}
2405+
23772406
/**
23782407
* Helper method creating a mocked token ECDSA keys.
23792408
*
@@ -2418,9 +2447,14 @@ public ICryptoTokenV4 getCryptoToken(final IServices services) {
24182447
* Mocked PDFSigner used to emulate allowlists for archiving and custom image paths.
24192448
*/
24202449
private static class MockedPathingPDFSigner extends PDFSigner {
2450+
2451+
public MockedPathingPDFSigner(ICryptoTokenV4 cryptoToken) {
2452+
this.cryptoToken = cryptoToken;
2453+
}
2454+
24212455
@Override
24222456
public ICryptoTokenV4 getCryptoToken(final IServices services) {
2423-
return null;
2457+
return cryptoToken;
24242458
}
24252459

24262460
@Override
@@ -2470,7 +2504,7 @@ public ICryptoTokenV4 getCryptoToken(final IServices services) {
24702504
final WorkerConfig config = new WorkerConfig();
24712505
final List<String> configErrors = new LinkedList<>();
24722506
config.setProperty("TSA_URL", "http://any-tsa.example.com");
2473-
final PDFSignerParameters params = new PDFSignerParameters(1234, config, configErrors, new HashMap<>(), new HashSet<>());
2507+
final PDFSignerParameters params = new PDFSignerParameters(1234, config, configErrors, new HashMap<>(), new HashSet<>(), Collections.emptySet());
24742508

24752509
instance.setIncludeCertificateLevels(1);
24762510

@@ -2583,12 +2617,7 @@ private void setupWorkers()
25832617
config.setProperty(AUTHTYPE, "NOAUTH");
25842618

25852619
workerMock.setupWorker(workerId, CRYPTOTOKEN_CLASSNAME, config,
2586-
new PDFSigner() {
2587-
@Override
2588-
public ICryptoTokenV4 getCryptoToken(final IServices services) {
2589-
return token;
2590-
}
2591-
});
2620+
new MockedPathingPDFSigner(token));
25922621
workerSession.reloadConfiguration(workerId);
25932622
}
25942623

@@ -2614,12 +2643,7 @@ public ICryptoTokenV4 getCryptoToken(final IServices services) {
26142643
config.setProperty(AUTHTYPE, "NOAUTH");
26152644

26162645
workerMock.setupWorker(workerId, CRYPTOTOKEN_CLASSNAME, config,
2617-
new PDFSigner() {
2618-
@Override
2619-
public ICryptoTokenV4 getCryptoToken(final IServices services) {
2620-
return tokenCRL;
2621-
}
2622-
});
2646+
new MockedPathingPDFSigner(tokenCRL));
26232647
workerSession.reloadConfiguration(workerId);
26242648
}
26252649
}
@@ -2731,7 +2755,7 @@ private static void assertOwnerPassword(byte[] pdfBytes, String password) throws
27312755
private static class PDFSignerBuilder {
27322756

27332757
private WorkerConfig workerConfig = new WorkerConfig();
2734-
private MockedPathingPDFSigner instance = new MockedPathingPDFSigner();
2758+
private MockedPathingPDFSigner instance = new MockedPathingPDFSigner(null);
27352759

27362760
public PDFSignerBuilder() {
27372761
workerConfig.setProperty("TYPE", "PROCESSABLE");

0 commit comments

Comments
 (0)