From f91164789e529f0a45c139dc3008a3ee734f1d80 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Thu, 4 Jun 2026 15:09:18 +0200 Subject: [PATCH 01/11] Fix URL signing api for URLs containing special characters --- .../fix-url-signing-special-characters.md | 25 +++++++ .../source/installation/config.rst | 7 +- docker-compose-dev.yml | 1 + .../edu/harvard/iq/dataverse/api/Admin.java | 9 ++- .../iq/dataverse/util/UrlSignerUtil.java | 54 ++++++++++----- .../iq/dataverse/util/UrlSignerUtilTest.java | 66 +++++++++++++++++++ 6 files changed, 143 insertions(+), 19 deletions(-) create mode 100644 doc/release-notes/fix-url-signing-special-characters.md diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md new file mode 100644 index 00000000000..48fff1ae79d --- /dev/null +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -0,0 +1,25 @@ +### Signed URLs work again for URLs with special characters + +Requesting a signed URL (e.g. via `/api/admin/requestSignedUrl`, used by external tools, the Globus +integration and third-party integrations) was broken for URLs whose query contained special +characters; most notably persistent IDs such as `doi:10.5072/FK2/ABC` (which contain `:` and +`/`), as well as spaces, percent-encoded values and non-ASCII characters. The signing logic had +started normalizing/re-encoding the URL before signing it, while the signature is a byte-exact MAC +over the URL string; the re-encoded bytes no longer matched what callers presented back, so +validation failed with "signature does not match"/authentication errors. + +Signing is now byte-exact again: the base URL is preserved exactly as provided (reserved signing +parameters are still stripped, but without re-encoding the rest of the URL). Clients must continue to +present the signed URL exactly as Dataverse returned it. + +### A signing secret is now required to request signed URLs + +The `/api/admin/requestSignedUrl` endpoint now requires a non-empty signing secret +(`dataverse.api.signing-secret`) to be configured. Previously an unset secret silently fell back to +using only the user's API token as the signing key, which is too weak. If the secret is not +configured, the endpoint now returns an error instead of issuing a weakly-signed URL. + +**Upgrade note:** installations that use signed URLs through this endpoint (including the +`rdm-integration` connector) must set `dataverse.api.signing-secret`. See the +[Configuration Guide](https://guides.dataverse.org/en/latest/installation/config.html#dataverse-api-signing-secret). +Treat the value like a password. diff --git a/doc/sphinx-guides/source/installation/config.rst b/doc/sphinx-guides/source/installation/config.rst index f2a6fdfa324..ef28b54c651 100644 --- a/doc/sphinx-guides/source/installation/config.rst +++ b/doc/sphinx-guides/source/installation/config.rst @@ -3349,9 +3349,10 @@ are time limited and only allow the action of the API call in the URL. See :ref: :ref:`api-native-signed-url` for more details. The key used to sign a URL is created from the API token of the creating user plus a signing-secret provided by an administrator. -**Using a signing-secret is highly recommended.** This setting defaults to an empty string. Using a non-empty -signing-secret makes it impossible for someone who knows an API token from forging signed URLs and provides extra security by -making the overall signing key longer. +**A non-empty signing-secret is required to request signed URLs through the API.** If it is not configured, the +``/api/admin/requestSignedUrl`` endpoint (see :ref:`api-native-signed-url`) returns an error instead of issuing a +weakly-signed URL. (The setting otherwise defaults to an empty string.) A non-empty signing-secret makes it impossible for +someone who only knows an API token to forge signed URLs, and provides extra security by making the overall signing key longer. **WARNING**: *Since the signing-secret is sensitive, you should treat it like a password.* diff --git a/docker-compose-dev.yml b/docker-compose-dev.yml index b24bf0ed6f6..86aff92e523 100644 --- a/docker-compose-dev.yml +++ b/docker-compose-dev.yml @@ -60,6 +60,7 @@ services: -Ddataverse.pid.fake.label=FakeDOIProvider -Ddataverse.pid.fake.authority=10.5072 -Ddataverse.pid.fake.shoulder=FK2/ + -Ddataverse.api.signing-secret=dev-only-signing-secret-change-me -Ddataverse.cors.origin=* \ -Ddataverse.cors.methods=GET,POST,PUT,DELETE,OPTIONS \ -Ddataverse.cors.headers.allow=range,content-type,x-dataverse-key,accept \ diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index 1eecdd4b171..031a8a6845e 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -2453,7 +2453,14 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur if (superuser == null || !superuser.isSuperuser()) { return error(Response.Status.FORBIDDEN, "Requesting signed URLs is restricted to superusers."); } - + + // A signing secret must be configured. Without it the signing key would fall back to just + // the user's API token, which is too weak. Fail fast rather than hand out weakly-signed URLs. + if (JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("").isEmpty()) { + return error(Response.Status.BAD_REQUEST, + "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); + } + String userId = urlInfo.getString("user"); String key=null; if (userId != null) { diff --git a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java index 222d265a0c3..49ae548c9d0 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java @@ -2,12 +2,10 @@ import org.apache.commons.codec.digest.DigestUtils; import org.apache.http.NameValuePair; -import org.apache.http.client.utils.URIBuilder; import org.apache.http.client.utils.URLEncodedUtils; import org.joda.time.LocalDateTime; import java.net.MalformedURLException; -import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.List; @@ -45,19 +43,13 @@ public class UrlSignerUtil { */ public static String signUrl(String baseUrl, Integer timeout, String user, String method, String key) { - // check for reserved parameter names ("until","user", "method", or "token") - String[] urlQP = baseUrl.split("\\?"); - if (urlQP.length > 1) { - try { - URIBuilder uriBuilder = new URIBuilder(baseUrl); - List params = uriBuilder.getQueryParams(); - params.removeIf(pair -> reservedParameters.contains(pair.getName())); - uriBuilder.setParameters(params); - baseUrl = uriBuilder.build().toString(); - } catch (URISyntaxException e) { - logger.severe("Invalid URL for signing: " + baseUrl + " " + e.getMessage()); - } - } + // Remove any reserved signing parameters ("until", "user", "method", "token", "key", + // "signed") that may already be present in the base URL, so they cannot be spoofed and so + // re-signing an already-signed URL does not accumulate them. This is done with exact string + // surgery (not URIBuilder) because the signature is a byte-exact MAC computed over the URL + // string: normalizing/re-encoding the URL here (e.g. percent-encoding ':' and '/' in DOIs, + // or handling spaces/unicode) would change the bytes that get hashed and break validation. + baseUrl = stripReservedParameters(baseUrl); boolean firstParam = !baseUrl.contains("?"); StringBuilder signedUrlBuilder = new StringBuilder(baseUrl); @@ -87,6 +79,38 @@ public static String signUrl(String baseUrl, Integer timeout, String user, Strin return signedUrl; } + /** + * Removes any reserved signing parameters ("until", "user", "method", "token", "key", + * "signed") from the query string of the given URL while preserving the exact byte + * representation of the path and of every remaining query parameter. Unlike URIBuilder, this + * does not decode/re-encode the URL, which is required because {@link #signUrl} and + * {@link #isValidUrl} compute a byte-exact MAC over the URL string. + * + * @param baseUrl the URL to clean (may or may not contain a query string) + * @return the URL with reserved parameters removed, otherwise unchanged byte-for-byte + */ + static String stripReservedParameters(String baseUrl) { + int queryStart = baseUrl.indexOf('?'); + if (queryStart < 0) { + return baseUrl; + } + String path = baseUrl.substring(0, queryStart); + String query = baseUrl.substring(queryStart + 1); + StringBuilder kept = new StringBuilder(); + for (String pair : query.split("&")) { + int equals = pair.indexOf('='); + String name = (equals < 0) ? pair : pair.substring(0, equals); + if (reservedParameters.contains(name)) { + continue; + } + if (kept.length() > 0) { + kept.append('&'); + } + kept.append(pair); + } + return kept.length() > 0 ? path + "?" + kept : path; + } + /** * This method will only return true if the URL and parameters except the * "token" are unchanged from the original/match the values sent to this method, diff --git a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java index d92f8822e59..36e05a80b70 100644 --- a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java @@ -8,6 +8,7 @@ import java.util.logging.Level; import java.util.logging.Logger; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -86,4 +87,69 @@ public void testSignAndValidateWithParams() { System.out.println(signedUrl3); assertTrue(signedUrl3.contains("&p2&")); // Show that this works with params that have no value } + + @Test + public void testStripReservedParametersPreservesSpecialCharacters() { + // The signature is a byte-exact MAC over the URL string, so removing the reserved signing + // params must not re-encode anything else: a DOI's ':' and '/' must survive unchanged. + String doiUrl = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123&foo=bar"; + assertEquals(doiUrl, UrlSignerUtil.stripReservedParameters(doiUrl)); + + // Reserved params (here token/user/signed) are removed; everything else is left byte-for-byte. + String withReserved = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123&token=spoofed&user=Mallory&signed=true&foo=bar"; + assertEquals("http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123&foo=bar", + UrlSignerUtil.stripReservedParameters(withReserved)); + + // A URL with no query string is returned unchanged. + String noQuery = "http://localhost:8080/api/v1/datasets/:persistentId"; + assertEquals(noQuery, UrlSignerUtil.stripReservedParameters(noQuery)); + } + + @Test + public void testSignAndValidateSpecialCharacters() { + final int longTimeout = 1000; + final String user = "Alice"; + final String method = "GET"; + final String key = "abracadabara open sesame"; + + // DOIs (':' and '/'), pre-encoded values, spaces, unicode and embedded URLs must all sign + // and then validate when the signed URL is presented back verbatim (as the server returns it). + String[] baseUrls = new String[] { + "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123&foo=bar", + "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi%3A10.5072%2FFK2%2FABC123", + "http://localhost:8080/api/v1/search?q=hello%20world&persistentId=doi:10.1/2", + "http://localhost:8080/api/v1/search?q=hello world&pid=doi:10.1/2", + "http://localhost:8080/api/v1/search?q=café&name=測試", + "http://localhost:8080/api/v1/redirect?url=http%3A%2F%2Fexample.com%2Ff%3Fa%3D1%26b%3D2" + }; + for (String baseUrl : baseUrls) { + String signedUrl = UrlSignerUtil.signUrl(baseUrl, longTimeout, user, method, key); + // The base URL is preserved byte-for-byte in the signed URL (no re-encoding). + assertTrue(signedUrl.startsWith(baseUrl + "&"), + "base URL must be preserved byte-for-byte: " + signedUrl); + assertTrue(UrlSignerUtil.isValidUrl(signedUrl, user, method, key), + "signed URL should validate when used verbatim: " + signedUrl); + } + } + + @Test + public void testSignedUrlIsByteExact() { + // Documents and locks in the byte-exact contract: the signature is computed over the URL + // string exactly as provided. A client that re-encodes the URL before presenting it back + // (e.g. percent-encoding the DOI) must fail validation. This is the regression that broke + // signing for special characters when the URL was normalized via URIBuilder. + final int longTimeout = 1000; + final String user = "Alice"; + final String method = "GET"; + final String key = "abracadabara open sesame"; + + String baseUrl = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123"; + String signedUrl = UrlSignerUtil.signUrl(baseUrl, longTimeout, user, method, key); + assertTrue(UrlSignerUtil.isValidUrl(signedUrl, user, method, key)); + + // Re-encoding ':' and '/' in the DOI changes the signed bytes, so it must be rejected. + String reEncoded = signedUrl.replace("doi:10.5072/FK2/ABC123", "doi%3A10.5072%2FFK2%2FABC123"); + assertFalse(UrlSignerUtil.isValidUrl(reEncoded, user, method, key), + "a re-encoded variant must not validate (byte-exact contract)"); + } } From 96be125110420321370f38365bc768798ca3579d Mon Sep 17 00:00:00 2001 From: ErykKul Date: Thu, 4 Jun 2026 15:47:32 +0200 Subject: [PATCH 02/11] Extra URL signing tests to prevent regression --- .../iq/dataverse/util/UrlSignerUtilTest.java | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java index 36e05a80b70..4ae6a8d3f79 100644 --- a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java @@ -152,4 +152,110 @@ public void testSignedUrlIsByteExact() { assertFalse(UrlSignerUtil.isValidUrl(reEncoded, user, method, key), "a re-encoded variant must not validate (byte-exact contract)"); } + + /** + * Signs {@code baseUrl} and asserts that the signed URL is exactly the provided URL followed by + * a well-formed signature (until/user/method/token), and that it validates when presented back + * verbatim. Returns the signed URL so callers can make further (encoding) assertions on it. + */ + private static String assertSignedExactlyAndValid(String baseUrl, String user, String key) { + String signed = UrlSignerUtil.signUrl(baseUrl, 1000, user, "GET", key); + // What is signed is exactly what was provided (no re-encoding/normalization of the base URL)... + assertTrue(signed.startsWith(baseUrl), + "what is signed must be exactly what was provided: " + baseUrl + "\n got: " + signed); + // ...plus only the signature parameters, appended after it. + String signature = signed.substring(baseUrl.length()); + String separator = baseUrl.contains("?") ? "&" : "\\?"; + assertTrue(signature.matches(separator + "until=[^&]+&user=" + user + "&method=GET&token=[0-9a-f]{128}"), + "only the signature parameters may be appended after the provided URL: " + signature); + // ...and the signature validates when the signed URL is used verbatim (as the client does). + assertTrue(UrlSignerUtil.isValidUrl(signed, user, "GET", key), + "signed URL must validate when presented back verbatim: " + signed); + return signed; + } + + @Test + public void testSignAndValidateRdmIntegrationUrls() { + // Backend regression guard: sign every URL shape that rdm-integration sends through the + // signing client and confirm (a) what is signed is exactly the URL provided plus a valid + // signature, (b) the signature validates verbatim, and (c) the encoding is preserved exactly + // (escaped values stay escaped, raw values stay raw). URL templates mirror rdm-integration: + // plugin/impl/globus/streams.go, dataverse/dataverse_read.go, dataverse/dataverse_write.go, + // plugin/impl/dataverse/query.go. If the backend ever re-encodes the URL again (the original + // regression), these assertions fail. + final String user = "rdmUser"; + final String key = "abracadabara open sesame"; + final String s = "https://demo.dataverse.org"; + final String pid = "doi:10.5072/FK2/ABC123"; // raw, as most rdm paths send it + final String escPid = "doi%3A10.5072%2FFK2%2FABC123"; // url.QueryEscape form + + // Paths that send the persistentId RAW (GetNodeMap, CheckPermission, all globus flows, + // all write flows, the dataverse plugin). The ':' and '/' in the DOI must survive unchanged. + String[] rawPidUrls = new String[] { + s + "/api/v1/datasets/:persistentId/versions/:latest/files?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId?persistentId=" + pid, + s + "/api/v1/admin/permissions/:persistentId?persistentId=" + pid + "&unblock-key=UNBLOCK", + s + "/api/v1/datasets/:persistentId/requestGlobusUploadPaths?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/addGlobusFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/requestGlobusDownload?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/monitorGlobusDownload?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/globusDownloadParameters?persistentId=" + pid + "&downloadId=globus-task-123", + s + "/api/v1/datasets/:persistentId/add?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/addFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/replaceFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/deleteFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/cleanStorage?persistentId=" + pid, + }; + for (String baseUrl : rawPidUrls) { + String signed = assertSignedExactlyAndValid(baseUrl, user, key); + assertTrue(signed.contains("persistentId=" + pid), + "raw persistentId must stay raw (not percent-encoded): " + signed); + assertFalse(signed.contains(escPid), + "raw persistentId must not be escaped by the backend: " + signed); + } + + // Paths that send the persistentId URL-ESCAPED (GetDatasetMetadata, GetDatasetUserPermissions). + // The escaped value must remain escaped (the backend must not decode then re-encode it). + String[] escapedPidUrls = new String[] { + s + "/api/v1/datasets/:persistentId?persistentId=" + escPid + "&excludeFiles=true", + s + "/api/v1/datasets/:persistentId/userPermissions?persistentId=" + escPid, + }; + for (String baseUrl : escapedPidUrls) { + String signed = assertSignedExactlyAndValid(baseUrl, user, key); + assertTrue(signed.contains("persistentId=" + escPid), + "escaped persistentId must stay escaped: " + signed); + assertFalse(signed.contains("persistentId=" + pid), + "escaped persistentId must not be decoded to raw: " + signed); + } + + // mydata/retrieve: URL-escaped search term, '+' for spaces, and repeated query parameters. + String mydata = s + "/api/v1/mydata/retrieve?selected_page=1&dvobject_types=Dataset" + + "&published_states=Published&published_states=Unpublished&published_states=Draft" + + "&role_ids=1&role_ids=6&mydata_search_term=text%3A%22hello+world%22"; + String signedMydata = assertSignedExactlyAndValid(mydata, user, key); + assertTrue(signedMydata.contains("mydata_search_term=text%3A%22hello+world%22"), + "escaped search term (including '%3A', '%22' and '+') must be preserved exactly: " + signedMydata); + assertTrue(signedMydata.contains("published_states=Published&published_states=Unpublished&published_states=Draft"), + "repeated query parameters must be preserved: " + signedMydata); + + // Numeric-id / no-persistentId paths (datafile, files, numeric dataset id, users/:me). + String[] otherUrls = new String[] { + s + "/api/v1/access/datafile/123/metadata/ddi", + s + "/api/v1/access/datafile/123", + s + "/api/v1/files/123", + s + "/api/v1/users/:me", + s + "/api/v1/datasets/42/versions/:latest?excludeFiles=true", + }; + for (String baseUrl : otherUrls) { + assertSignedExactlyAndValid(baseUrl, user, key); + } + + // noSlashPermissionUrl sends an empty leading query segment ("?&unblock-key=..."); the backend + // drops the empty segment, so it does not round-trip byte-for-byte, but it still signs and + // validates (the client uses the returned signed URL verbatim). + String emptySegment = s + "/api/v1/admin/permissions/42?&unblock-key=UNBLOCK"; + String signedEmptySegment = UrlSignerUtil.signUrl(emptySegment, 1000, user, "GET", key); + assertTrue(UrlSignerUtil.isValidUrl(signedEmptySegment, user, "GET", key), + "permissions URL with an empty leading query segment must validate: " + signedEmptySegment); + } } From 88125820759a05f022de1a55b847393d7dfe8c29 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Thu, 4 Jun 2026 16:29:14 +0200 Subject: [PATCH 03/11] More URL signing tests --- .../api/auth/SignedUrlAuthMechanismTest.java | 59 +++++++++++++++++++ .../SignedUrlContainerRequestTestFake.java | 6 +- .../doubles/SignedUrlUriInfoTestFake.java | 12 ++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java index 6fd7d2e1d8e..4ccc0b8c90c 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java @@ -5,6 +5,7 @@ import edu.harvard.iq.dataverse.authorization.users.ApiToken; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; import edu.harvard.iq.dataverse.authorization.users.User; +import edu.harvard.iq.dataverse.util.UrlSignerUtil; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -96,4 +97,62 @@ public void testFindUserFromRequest_SignedUrlTokenProvided_UserDoesNotExistForTh assertEquals(RESPONSE_MESSAGE_BAD_SIGNED_URL, wrappedUnauthorizedAuthErrorResponse.getMessage()); } + + // ---- End-to-end signature validation through the REAL SignedUrlAuthMechanism ---- + // These drive the actual validation path: sign a URL, then validate it the way the server does + // (URLDecoder.decode(requestUri) -> UrlSignerUtil.isValidUrl), proving a signed URL produced for + // an rdm-integration-style URL actually authenticates the user (and that tampering does not). + // No signing-secret is configured in this unit context, so the signing key is just the API token. + + private void givenUserWithSigningKey(String key) { + AuthenticationServiceBean authStub = Mockito.mock(AuthenticationServiceBean.class); + Mockito.when(authStub.getAuthenticatedUser(TEST_SIGNED_URL_USER_ID)).thenReturn(testAuthenticatedUser); + ApiToken apiToken = Mockito.mock(ApiToken.class); + Mockito.when(apiToken.getTokenString()).thenReturn(key); + Mockito.when(authStub.findApiTokenByUser(testAuthenticatedUser)).thenReturn(apiToken); + sut.authSvc = authStub; + } + + @Test + public void testEndToEnd_rawPersistentIdSignedUrl_authenticatesUser() throws WrappedAuthErrorResponse { + givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); + String base = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; + // The client signs this URL and requests it verbatim (a raw DOI is unchanged by URLDecoder.decode). + String signedUrl = UrlSignerUtil.signUrl(base, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + + ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, signedUrl); + + assertEquals(testAuthenticatedUser, sut.findUserFromRequest(request), + "a signed raw-DOI URL must authenticate end to end (decode + validate)"); + } + + @Test + public void testEndToEnd_escapedPersistentIdReconstructedRequest_authenticatesUser() throws WrappedAuthErrorResponse { + givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); + // The signing client un-escapes before signing, so the server signs the DECODED form... + String decodedBase = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; + String signedDecoded = UrlSignerUtil.signUrl(decodedBase, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + // ...but the actual request carries the ESCAPED persistentId (the URL the caller built). The + // server's URLDecoder.decode turns it back into the signed (decoded) form, so it validates. + String escapedBase = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi%3A10.5072%2FFK2%2FABC"; + String requestUri = escapedBase + signedDecoded.substring(decodedBase.length()); + + ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, requestUri); + + assertEquals(testAuthenticatedUser, sut.findUserFromRequest(request), + "an escaped persistentId must authenticate end to end (server signs decoded; validation decodes the request)"); + } + + @Test + public void testEndToEnd_tamperedSignedUrl_userNotAuthenticated() { + givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); + String base = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; + String signedUrl = UrlSignerUtil.signUrl(base, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + // Alter the signed portion of the URL after signing -> the signature must no longer validate. + String tampered = signedUrl.replace("FK2/ABC", "FK2/HACKED"); + + ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, tampered); + + assertThrows(WrappedUnauthorizedAuthErrorResponse.class, () -> sut.findUserFromRequest(request)); + } } diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java index df37f6723d3..6a1a440f713 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java @@ -6,7 +6,11 @@ public class SignedUrlContainerRequestTestFake extends ContainerRequestTestFake private final UriInfo uriInfo; public SignedUrlContainerRequestTestFake(String signedUrlToken, String signedUrlUserId) { - this.uriInfo = new SignedUrlUriInfoTestFake(signedUrlToken, signedUrlUserId); + this(signedUrlToken, signedUrlUserId, null); + } + + public SignedUrlContainerRequestTestFake(String signedUrlToken, String signedUrlUserId, String requestUriOverride) { + this.uriInfo = new SignedUrlUriInfoTestFake(signedUrlToken, signedUrlUserId, requestUriOverride); } @Override diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java index fa9da7fc8de..1ac3f44ac17 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java @@ -15,18 +15,30 @@ public class SignedUrlUriInfoTestFake extends UriInfoTestFake { private final String signedUrlToken; private final String signedUrlUserId; + private final String requestUriOverride; private static final String SIGNED_URL_BASE_URL = "http://localhost:8080/api/test1"; private static final Integer SIGNED_URL_TIMEOUT = 1000; public SignedUrlUriInfoTestFake(String signedUrlToken, String signedUrlUserId) { + this(signedUrlToken, signedUrlUserId, null); + } + + // Lets a test supply the exact request URI the server would see (e.g. a signed URL for a + // specific base URL, or a reconstructed/escaped presentation), so the real validation path + // (URLDecoder.decode + UrlSignerUtil.isValidUrl) can be exercised end to end. + public SignedUrlUriInfoTestFake(String signedUrlToken, String signedUrlUserId, String requestUriOverride) { this.signedUrlToken = signedUrlToken; this.signedUrlUserId = signedUrlUserId; + this.requestUriOverride = requestUriOverride; } @Override public URI getRequestUri() { + if (requestUriOverride != null) { + return URI.create(requestUriOverride); + } return URI.create(UrlSignerUtil.signUrl(SIGNED_URL_BASE_URL, SIGNED_URL_TIMEOUT, signedUrlUserId, GET, signedUrlToken)); } From 9cb852a4aac5ade984b13c3ec3f10ec06c403144 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Thu, 4 Jun 2026 20:21:11 +0200 Subject: [PATCH 04/11] Clean up URL signing tests and comments --- .../edu/harvard/iq/dataverse/api/Admin.java | 3 +- .../iq/dataverse/util/UrlSignerUtil.java | 19 +-- .../api/auth/SignedUrlAuthMechanismTest.java | 94 +++++++++----- .../doubles/SignedUrlUriInfoTestFake.java | 5 +- .../iq/dataverse/util/UrlSignerUtilTest.java | 115 +----------------- 5 files changed, 73 insertions(+), 163 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index 031a8a6845e..91d932d12d6 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -2454,8 +2454,7 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur return error(Response.Status.FORBIDDEN, "Requesting signed URLs is restricted to superusers."); } - // A signing secret must be configured. Without it the signing key would fall back to just - // the user's API token, which is too weak. Fail fast rather than hand out weakly-signed URLs. + // Require a signing secret: without it the key is only the user's API token, which is too weak. if (JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("").isEmpty()) { return error(Response.Status.BAD_REQUEST, "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); diff --git a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java index 49ae548c9d0..9bc39598dbb 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java @@ -43,12 +43,9 @@ public class UrlSignerUtil { */ public static String signUrl(String baseUrl, Integer timeout, String user, String method, String key) { - // Remove any reserved signing parameters ("until", "user", "method", "token", "key", - // "signed") that may already be present in the base URL, so they cannot be spoofed and so - // re-signing an already-signed URL does not accumulate them. This is done with exact string - // surgery (not URIBuilder) because the signature is a byte-exact MAC computed over the URL - // string: normalizing/re-encoding the URL here (e.g. percent-encoding ':' and '/' in DOIs, - // or handling spaces/unicode) would change the bytes that get hashed and break validation. + // Strip reserved signing params that may already be in the base URL. Done with exact-string + // surgery (not URIBuilder): the signature is a byte-exact MAC, so re-encoding the URL here + // (e.g. percent-encoding ':' and '/' in DOIs) would change the hashed bytes and break it. baseUrl = stripReservedParameters(baseUrl); boolean firstParam = !baseUrl.contains("?"); StringBuilder signedUrlBuilder = new StringBuilder(baseUrl); @@ -80,14 +77,8 @@ public static String signUrl(String baseUrl, Integer timeout, String user, Strin } /** - * Removes any reserved signing parameters ("until", "user", "method", "token", "key", - * "signed") from the query string of the given URL while preserving the exact byte - * representation of the path and of every remaining query parameter. Unlike URIBuilder, this - * does not decode/re-encode the URL, which is required because {@link #signUrl} and - * {@link #isValidUrl} compute a byte-exact MAC over the URL string. - * - * @param baseUrl the URL to clean (may or may not contain a query string) - * @return the URL with reserved parameters removed, otherwise unchanged byte-for-byte + * Removes the reserved signing parameters from the query, preserving the exact bytes of the path + * and of every other parameter (unlike URIBuilder, which would re-encode and break the MAC). */ static String stripReservedParameters(String baseUrl) { int queryStart = baseUrl.indexOf('?'); diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java index 4ccc0b8c90c..076894f4b10 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanismTest.java @@ -12,6 +12,10 @@ import jakarta.ws.rs.container.ContainerRequestContext; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; +import java.util.List; + import static edu.harvard.iq.dataverse.api.auth.SignedUrlAuthMechanism.RESPONSE_MESSAGE_BAD_SIGNED_URL; import static org.junit.jupiter.api.Assertions.*; @@ -98,11 +102,9 @@ public void testFindUserFromRequest_SignedUrlTokenProvided_UserDoesNotExistForTh assertEquals(RESPONSE_MESSAGE_BAD_SIGNED_URL, wrappedUnauthorizedAuthErrorResponse.getMessage()); } - // ---- End-to-end signature validation through the REAL SignedUrlAuthMechanism ---- - // These drive the actual validation path: sign a URL, then validate it the way the server does - // (URLDecoder.decode(requestUri) -> UrlSignerUtil.isValidUrl), proving a signed URL produced for - // an rdm-integration-style URL actually authenticates the user (and that tampering does not). - // No signing-secret is configured in this unit context, so the signing key is just the API token. + // End-to-end validation through the REAL SignedUrlAuthMechanism (URLDecoder.decode + isValidUrl), + // which the isValidUrl-only tests in UrlSignerUtilTest do not exercise. No signing secret is + // configured here, so the signing key is just the API token. private void givenUserWithSigningKey(String key) { AuthenticationServiceBean authStub = Mockito.mock(AuthenticationServiceBean.class); @@ -114,45 +116,71 @@ private void givenUserWithSigningKey(String key) { } @Test - public void testEndToEnd_rawPersistentIdSignedUrl_authenticatesUser() throws WrappedAuthErrorResponse { + public void testEndToEnd_tamperedSignedUrl_userNotAuthenticated() { givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); String base = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; - // The client signs this URL and requests it verbatim (a raw DOI is unchanged by URLDecoder.decode). String signedUrl = UrlSignerUtil.signUrl(base, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + // Alter the signed portion of the URL after signing -> the signature must no longer validate. + String tampered = signedUrl.replace("FK2/ABC", "FK2/HACKED"); - ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, signedUrl); + ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, tampered); - assertEquals(testAuthenticatedUser, sut.findUserFromRequest(request), - "a signed raw-DOI URL must authenticate end to end (decode + validate)"); + assertThrows(WrappedUnauthorizedAuthErrorResponse.class, () -> sut.findUserFromRequest(request)); } - @Test - public void testEndToEnd_escapedPersistentIdReconstructedRequest_authenticatesUser() throws WrappedAuthErrorResponse { + // Runs the real rdm flow: un-escape, sign, request the original (encoded) URL + signature, then the + // server URL-decodes the request and checks it. Returns true iff the user authenticates. + private boolean validatesEndToEndAsRdmClient(String urlAsClientBuilds) { givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); - // The signing client un-escapes before signing, so the server signs the DECODED form... - String decodedBase = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; - String signedDecoded = UrlSignerUtil.signUrl(decodedBase, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); - // ...but the actual request carries the ESCAPED persistentId (the URL the caller built). The - // server's URLDecoder.decode turns it back into the signed (decoded) form, so it validates. - String escapedBase = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi%3A10.5072%2FFK2%2FABC"; - String requestUri = escapedBase + signedDecoded.substring(decodedBase.length()); - + String canonical = URLDecoder.decode(urlAsClientBuilds, StandardCharsets.UTF_8); + String signed = UrlSignerUtil.signUrl(canonical, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + String requestUri = urlAsClientBuilds + signed.substring(canonical.length()); ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, requestUri); - - assertEquals(testAuthenticatedUser, sut.findUserFromRequest(request), - "an escaped persistentId must authenticate end to end (server signs decoded; validation decodes the request)"); + try { + return testAuthenticatedUser.equals(sut.findUserFromRequest(request)); + } catch (WrappedAuthErrorResponse e) { + return false; + } } @Test - public void testEndToEnd_tamperedSignedUrl_userNotAuthenticated() { - givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); - String base = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC"; - String signedUrl = UrlSignerUtil.signUrl(base, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); - // Alter the signed portion of the URL after signing -> the signature must no longer validate. - String tampered = signedUrl.replace("FK2/ABC", "FK2/HACKED"); - - ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, tampered); - - assertThrows(WrappedUnauthorizedAuthErrorResponse.class, () -> sut.findUserFromRequest(request)); + public void testEndToEnd_allRdmIntegrationUrls_authenticate() { + final String s = "https://demo.dataverse.org"; + final String pid = "doi:10.5072/FK2/ABC"; // raw, as most rdm paths send it + final String escPid = "doi%3A10.5072%2FFK2%2FABC"; // url.QueryEscape form (GetDatasetMetadata, GetDatasetUserPermissions) + + // Every URL shape rdm-integration signs - each must authenticate end to end. + List urls = List.of( + // raw persistentId in the query (GetNodeMap, CheckPermission, globus, writes, dataverse plugin) + s + "/api/v1/datasets/:persistentId/versions/:latest/files?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId?persistentId=" + pid, + s + "/api/v1/admin/permissions/:persistentId?persistentId=" + pid + "&unblock-key=UNBLOCK", + s + "/api/v1/datasets/:persistentId/requestGlobusUploadPaths?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/addGlobusFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/requestGlobusDownload?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/monitorGlobusDownload?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/globusDownloadParameters?persistentId=" + pid + "&downloadId=globus-task-123", + s + "/api/v1/datasets/:persistentId/add?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/addFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/replaceFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/deleteFiles?persistentId=" + pid, + s + "/api/v1/datasets/:persistentId/cleanStorage?persistentId=" + pid, + // url-escaped persistentId (GetDatasetMetadata, GetDatasetUserPermissions) + s + "/api/v1/datasets/:persistentId?persistentId=" + escPid + "&excludeFiles=true", + s + "/api/v1/datasets/:persistentId/userPermissions?persistentId=" + escPid, + // mydata/retrieve: url-escaped search term, '+' for spaces, repeated query params + s + "/api/v1/mydata/retrieve?selected_page=1&dvobject_types=Dataset" + + "&published_states=Published&published_states=Unpublished&published_states=Draft" + + "&role_ids=1&role_ids=6&mydata_search_term=text%3A%22hello+world%22", + // numeric-id / no-persistentId paths + s + "/api/v1/access/datafile/123/metadata/ddi", + s + "/api/v1/access/datafile/123", + s + "/api/v1/files/123", + s + "/api/v1/users/:me", + s + "/api/v1/datasets/42/versions/:latest?excludeFiles=true" + ); + for (String url : urls) { + assertTrue(validatesEndToEndAsRdmClient(url), "signed URL must authenticate end to end: " + url); + } } } diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java index 1ac3f44ac17..5edd31509f8 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java @@ -25,9 +25,8 @@ public SignedUrlUriInfoTestFake(String signedUrlToken, String signedUrlUserId) { this(signedUrlToken, signedUrlUserId, null); } - // Lets a test supply the exact request URI the server would see (e.g. a signed URL for a - // specific base URL, or a reconstructed/escaped presentation), so the real validation path - // (URLDecoder.decode + UrlSignerUtil.isValidUrl) can be exercised end to end. + // Lets a test supply the exact request URI the server would see, to exercise the real + // URLDecoder.decode + isValidUrl validation path. public SignedUrlUriInfoTestFake(String signedUrlToken, String signedUrlUserId, String requestUriOverride) { this.signedUrlToken = signedUrlToken; this.signedUrlUserId = signedUrlUserId; diff --git a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java index 4ae6a8d3f79..94b76a9678b 100644 --- a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java @@ -113,7 +113,8 @@ public void testSignAndValidateSpecialCharacters() { final String key = "abracadabara open sesame"; // DOIs (':' and '/'), pre-encoded values, spaces, unicode and embedded URLs must all sign - // and then validate when the signed URL is presented back verbatim (as the server returns it). + // byte-exact and be accepted by isValidUrl over those exact bytes (the signing primitive; + // end-to-end validation with the server's URLDecoder.decode is in SignedUrlAuthMechanismTest). String[] baseUrls = new String[] { "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123&foo=bar", "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi%3A10.5072%2FFK2%2FABC123", @@ -134,10 +135,8 @@ public void testSignAndValidateSpecialCharacters() { @Test public void testSignedUrlIsByteExact() { - // Documents and locks in the byte-exact contract: the signature is computed over the URL - // string exactly as provided. A client that re-encodes the URL before presenting it back - // (e.g. percent-encoding the DOI) must fail validation. This is the regression that broke - // signing for special characters when the URL was normalized via URIBuilder. + // Byte-exact contract: the signature is over the URL as provided, so a re-encoded variant + // must fail validation. This is the regression that URIBuilder normalization caused. final int longTimeout = 1000; final String user = "Alice"; final String method = "GET"; @@ -152,110 +151,4 @@ public void testSignedUrlIsByteExact() { assertFalse(UrlSignerUtil.isValidUrl(reEncoded, user, method, key), "a re-encoded variant must not validate (byte-exact contract)"); } - - /** - * Signs {@code baseUrl} and asserts that the signed URL is exactly the provided URL followed by - * a well-formed signature (until/user/method/token), and that it validates when presented back - * verbatim. Returns the signed URL so callers can make further (encoding) assertions on it. - */ - private static String assertSignedExactlyAndValid(String baseUrl, String user, String key) { - String signed = UrlSignerUtil.signUrl(baseUrl, 1000, user, "GET", key); - // What is signed is exactly what was provided (no re-encoding/normalization of the base URL)... - assertTrue(signed.startsWith(baseUrl), - "what is signed must be exactly what was provided: " + baseUrl + "\n got: " + signed); - // ...plus only the signature parameters, appended after it. - String signature = signed.substring(baseUrl.length()); - String separator = baseUrl.contains("?") ? "&" : "\\?"; - assertTrue(signature.matches(separator + "until=[^&]+&user=" + user + "&method=GET&token=[0-9a-f]{128}"), - "only the signature parameters may be appended after the provided URL: " + signature); - // ...and the signature validates when the signed URL is used verbatim (as the client does). - assertTrue(UrlSignerUtil.isValidUrl(signed, user, "GET", key), - "signed URL must validate when presented back verbatim: " + signed); - return signed; - } - - @Test - public void testSignAndValidateRdmIntegrationUrls() { - // Backend regression guard: sign every URL shape that rdm-integration sends through the - // signing client and confirm (a) what is signed is exactly the URL provided plus a valid - // signature, (b) the signature validates verbatim, and (c) the encoding is preserved exactly - // (escaped values stay escaped, raw values stay raw). URL templates mirror rdm-integration: - // plugin/impl/globus/streams.go, dataverse/dataverse_read.go, dataverse/dataverse_write.go, - // plugin/impl/dataverse/query.go. If the backend ever re-encodes the URL again (the original - // regression), these assertions fail. - final String user = "rdmUser"; - final String key = "abracadabara open sesame"; - final String s = "https://demo.dataverse.org"; - final String pid = "doi:10.5072/FK2/ABC123"; // raw, as most rdm paths send it - final String escPid = "doi%3A10.5072%2FFK2%2FABC123"; // url.QueryEscape form - - // Paths that send the persistentId RAW (GetNodeMap, CheckPermission, all globus flows, - // all write flows, the dataverse plugin). The ':' and '/' in the DOI must survive unchanged. - String[] rawPidUrls = new String[] { - s + "/api/v1/datasets/:persistentId/versions/:latest/files?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId?persistentId=" + pid, - s + "/api/v1/admin/permissions/:persistentId?persistentId=" + pid + "&unblock-key=UNBLOCK", - s + "/api/v1/datasets/:persistentId/requestGlobusUploadPaths?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/addGlobusFiles?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/requestGlobusDownload?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/monitorGlobusDownload?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/globusDownloadParameters?persistentId=" + pid + "&downloadId=globus-task-123", - s + "/api/v1/datasets/:persistentId/add?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/addFiles?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/replaceFiles?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/deleteFiles?persistentId=" + pid, - s + "/api/v1/datasets/:persistentId/cleanStorage?persistentId=" + pid, - }; - for (String baseUrl : rawPidUrls) { - String signed = assertSignedExactlyAndValid(baseUrl, user, key); - assertTrue(signed.contains("persistentId=" + pid), - "raw persistentId must stay raw (not percent-encoded): " + signed); - assertFalse(signed.contains(escPid), - "raw persistentId must not be escaped by the backend: " + signed); - } - - // Paths that send the persistentId URL-ESCAPED (GetDatasetMetadata, GetDatasetUserPermissions). - // The escaped value must remain escaped (the backend must not decode then re-encode it). - String[] escapedPidUrls = new String[] { - s + "/api/v1/datasets/:persistentId?persistentId=" + escPid + "&excludeFiles=true", - s + "/api/v1/datasets/:persistentId/userPermissions?persistentId=" + escPid, - }; - for (String baseUrl : escapedPidUrls) { - String signed = assertSignedExactlyAndValid(baseUrl, user, key); - assertTrue(signed.contains("persistentId=" + escPid), - "escaped persistentId must stay escaped: " + signed); - assertFalse(signed.contains("persistentId=" + pid), - "escaped persistentId must not be decoded to raw: " + signed); - } - - // mydata/retrieve: URL-escaped search term, '+' for spaces, and repeated query parameters. - String mydata = s + "/api/v1/mydata/retrieve?selected_page=1&dvobject_types=Dataset" - + "&published_states=Published&published_states=Unpublished&published_states=Draft" - + "&role_ids=1&role_ids=6&mydata_search_term=text%3A%22hello+world%22"; - String signedMydata = assertSignedExactlyAndValid(mydata, user, key); - assertTrue(signedMydata.contains("mydata_search_term=text%3A%22hello+world%22"), - "escaped search term (including '%3A', '%22' and '+') must be preserved exactly: " + signedMydata); - assertTrue(signedMydata.contains("published_states=Published&published_states=Unpublished&published_states=Draft"), - "repeated query parameters must be preserved: " + signedMydata); - - // Numeric-id / no-persistentId paths (datafile, files, numeric dataset id, users/:me). - String[] otherUrls = new String[] { - s + "/api/v1/access/datafile/123/metadata/ddi", - s + "/api/v1/access/datafile/123", - s + "/api/v1/files/123", - s + "/api/v1/users/:me", - s + "/api/v1/datasets/42/versions/:latest?excludeFiles=true", - }; - for (String baseUrl : otherUrls) { - assertSignedExactlyAndValid(baseUrl, user, key); - } - - // noSlashPermissionUrl sends an empty leading query segment ("?&unblock-key=..."); the backend - // drops the empty segment, so it does not round-trip byte-for-byte, but it still signs and - // validates (the client uses the returned signed URL verbatim). - String emptySegment = s + "/api/v1/admin/permissions/42?&unblock-key=UNBLOCK"; - String signedEmptySegment = UrlSignerUtil.signUrl(emptySegment, 1000, user, "GET", key); - assertTrue(UrlSignerUtil.isValidUrl(signedEmptySegment, user, "GET", key), - "permissions URL with an empty leading query segment must validate: " + signedEmptySegment); - } } From 942f3cf3ffc63ab7c93b23adaeb0423fe58a508a Mon Sep 17 00:00:00 2001 From: ErykKul Date: Mon, 8 Jun 2026 15:57:40 +0200 Subject: [PATCH 05/11] updated the release note --- doc/release-notes/fix-url-signing-special-characters.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index 48fff1ae79d..f60763f2d05 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -22,4 +22,6 @@ configured, the endpoint now returns an error instead of issuing a weakly-signed **Upgrade note:** installations that use signed URLs through this endpoint (including the `rdm-integration` connector) must set `dataverse.api.signing-secret`. See the [Configuration Guide](https://guides.dataverse.org/en/latest/installation/config.html#dataverse-api-signing-secret). -Treat the value like a password. +Treat the value like a password. Because the signing secret is part of the signing key, setting (or +later changing) it invalidates previously issued signed URLs: any existing signed URLs that have not +yet expired will stop working, and clients/integrations will need to request new ones. From 4387ee97d05bd687280f6a4f9b76ebe9cd17447e Mon Sep 17 00:00:00 2001 From: ErykKul Date: Mon, 8 Jun 2026 16:33:52 +0200 Subject: [PATCH 06/11] Clarify URL-signing release note and comment --- .../fix-url-signing-special-characters.md | 33 +++++++++++-------- .../iq/dataverse/util/UrlSignerUtil.java | 8 +++-- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index f60763f2d05..b547d64c540 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -1,23 +1,30 @@ ### Signed URLs work again for URLs with special characters Requesting a signed URL (e.g. via `/api/admin/requestSignedUrl`, used by external tools, the Globus -integration and third-party integrations) was broken for URLs whose query contained special -characters; most notably persistent IDs such as `doi:10.5072/FK2/ABC` (which contain `:` and -`/`), as well as spaces, percent-encoded values and non-ASCII characters. The signing logic had -started normalizing/re-encoding the URL before signing it, while the signature is a byte-exact MAC -over the URL string; the re-encoded bytes no longer matched what callers presented back, so -validation failed with "signature does not match"/authentication errors. +integration and third-party integrations such as the `rdm-integration` connector) was broken in 6.10 +for URLs whose query contained special characters — most notably persistent IDs such as +`doi:10.5072/FK2/ABC` (which contain `:` and `/`), as well as spaces, percent-encoded values and +non-ASCII characters. In 6.10 the signing step began re-encoding/normalizing the URL (for example +percent-encoding `:` and `/`) before computing the signature, while the request is validated against +the URL the caller actually presents back. The re-encoded signature no longer matched, so validation +failed with authentication / "signature does not match" errors. -Signing is now byte-exact again: the base URL is preserved exactly as provided (reserved signing -parameters are still stripped, but without re-encoding the rest of the URL). Clients must continue to -present the signed URL exactly as Dataverse returned it. +Signing no longer re-encodes the URL: it is signed exactly as provided, with only the reserved +signing parameters (`until`, `user`, `method`, `token`, `key`, `signed`) stripped out; the rest of +the URL is left untouched, character for character. + +**This restores the URL-signing behavior used before 6.10, so it is compatible with older versions +and with existing integrations.** Clients and connectors that build or consume signed URLs the way +they did before 6.10 keep working unchanged, signatures are computed the same way as before the +regression, and URLs containing special characters validate again. No client-side changes are +required. ### A signing secret is now required to request signed URLs -The `/api/admin/requestSignedUrl` endpoint now requires a non-empty signing secret -(`dataverse.api.signing-secret`) to be configured. Previously an unset secret silently fell back to -using only the user's API token as the signing key, which is too weak. If the secret is not -configured, the endpoint now returns an error instead of issuing a weakly-signed URL. +Separately from the fix above, the `/api/admin/requestSignedUrl` endpoint now requires a non-empty +signing secret (`dataverse.api.signing-secret`) to be configured. Previously an unset secret silently +fell back to using only the user's API token as the signing key, which is too weak. If the secret is +not configured, the endpoint now returns an error instead of issuing a weakly-signed URL. **Upgrade note:** installations that use signed URLs through this endpoint (including the `rdm-integration` connector) must set `dataverse.api.signing-secret`. See the diff --git a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java index 9bc39598dbb..2a918f5fb16 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java @@ -43,9 +43,11 @@ public class UrlSignerUtil { */ public static String signUrl(String baseUrl, Integer timeout, String user, String method, String key) { - // Strip reserved signing params that may already be in the base URL. Done with exact-string - // surgery (not URIBuilder): the signature is a byte-exact MAC, so re-encoding the URL here - // (e.g. percent-encoding ':' and '/' in DOIs) would change the hashed bytes and break it. + // Strip reserved signing params that may already be in the base URL, using exact-string + // surgery rather than URIBuilder. The URL must be signed exactly as provided (the pre-6.10 + // behavior): validation reconstructs the signing string from the URL-decoded request, so + // re-encoding here (e.g. percent-encoding ':' and '/' in DOIs) would change the signed bytes + // and the signature would no longer match. baseUrl = stripReservedParameters(baseUrl); boolean firstParam = !baseUrl.contains("?"); StringBuilder signedUrlBuilder = new StringBuilder(baseUrl); From 8b1ab7b06b998f89be624070e39ad4cdef8f1b54 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Mon, 8 Jun 2026 16:43:02 +0200 Subject: [PATCH 07/11] Return 500 instead of 400 when signing secret is not configured --- src/main/java/edu/harvard/iq/dataverse/api/Admin.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index 9985e60de70..e1ff9bb54fd 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -2456,7 +2456,7 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur // Require a signing secret: without it the key is only the user's API token, which is too weak. if (JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("").isEmpty()) { - return error(Response.Status.BAD_REQUEST, + return error(Response.Status.INTERNAL_SERVER_ERROR, "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); } From 4e8b6ff9b120c16257de485ebafdb709d17c6d58 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Tue, 16 Jun 2026 11:31:36 +0200 Subject: [PATCH 08/11] Require signing secret for guestbook-response signed download URLs too #12435 --- .../fix-url-signing-special-characters.md | 13 ++++++++----- .../java/edu/harvard/iq/dataverse/api/Access.java | 6 ++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index b547d64c540..f2f3b8476f8 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -21,13 +21,16 @@ required. ### A signing secret is now required to request signed URLs -Separately from the fix above, the `/api/admin/requestSignedUrl` endpoint now requires a non-empty +Separately from the fix above, the endpoints that issue signed URLs - `/api/admin/requestSignedUrl` +and the guestbook-response file download (`POST /api/access/datafile/{id}`) - now require a non-empty signing secret (`dataverse.api.signing-secret`) to be configured. Previously an unset secret silently -fell back to using only the user's API token as the signing key, which is too weak. If the secret is -not configured, the endpoint now returns an error instead of issuing a weakly-signed URL. +fell back to using only the user's API token (or, for a guest, a guessable value derived from the URL) +as the signing key, which is too weak. If the secret is not configured, these endpoints now return an +error instead of issuing a weakly-signed URL. -**Upgrade note:** installations that use signed URLs through this endpoint (including the -`rdm-integration` connector) must set `dataverse.api.signing-secret`. See the +**Upgrade note:** installations that use signed URLs through these endpoints (including the +`rdm-integration` connector and signed guestbook-response downloads) must set +`dataverse.api.signing-secret`. See the [Configuration Guide](https://guides.dataverse.org/en/latest/installation/config.html#dataverse-api-signing-secret). Treat the value like a password. Because the signing secret is part of the signing key, setting (or later changing) it invalidates previously issued signed URLs: any existing signed URLs that have not diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index a2d7d3ed525..ebecb30f93b 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -529,6 +529,12 @@ private Map getDatafilesMap(DataverseRequest req, String fileIds } private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, User user, String id, String gbrids) { + // Require a signing secret: without it the key is only the user's API token (or, for a guest, + // a guessable value derived from the URL), which is too weak. Mirrors Admin.getSignedUrl. + if (JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("").isEmpty()) { + return error(INTERNAL_SERVER_ERROR, + "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); + } // Create the signed URL String userIdentifier = null; String key = null; From 77ba0c8be664031897ea65f5c95a5f4d167c990b Mon Sep 17 00:00:00 2001 From: ErykKul Date: Tue, 16 Jun 2026 13:29:29 +0200 Subject: [PATCH 09/11] Centralize signing-secret requirement across all API-token-based URL signing #12435 --- .../fix-url-signing-special-characters.md | 25 +++++++++++---- .../dataverse/ManageFilePermissionsPage.java | 6 ++-- .../iq/dataverse/ManagePermissionsPage.java | 6 ++-- .../edu/harvard/iq/dataverse/api/Access.java | 10 ++++-- .../edu/harvard/iq/dataverse/api/Admin.java | 10 +++--- .../externaltools/ExternalToolHandler.java | 9 ++++-- .../dataverse/globus/GlobusServiceBean.java | 11 ++++--- .../iq/dataverse/util/URLTokenUtil.java | 10 +++--- .../iq/dataverse/util/UrlSignerUtil.java | 32 +++++++++++++++++++ .../ExternalToolHandlerTest.java | 1 + 10 files changed, 84 insertions(+), 36 deletions(-) diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index b547d64c540..d7a2853dfa2 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -19,15 +19,26 @@ they did before 6.10 keep working unchanged, signatures are computed the same wa regression, and URLs containing special characters validate again. No client-side changes are required. -### A signing secret is now required to request signed URLs +### A signing secret is now required for signed URLs -Separately from the fix above, the `/api/admin/requestSignedUrl` endpoint now requires a non-empty -signing secret (`dataverse.api.signing-secret`) to be configured. Previously an unset secret silently -fell back to using only the user's API token as the signing key, which is too weak. If the secret is -not configured, the endpoint now returns an error instead of issuing a weakly-signed URL. +Separately from the fix above, Dataverse no longer falls back to a weak signing key when +`dataverse.api.signing-secret` is unset. Previously, with no secret configured, signed URLs were +signed using only the user's API token (or, for a guest, a value derived from the public URL), which +is too weak to be a signing key. A non-empty `dataverse.api.signing-secret` is now required wherever +URLs are signed with a key based on a user's API token: -**Upgrade note:** installations that use signed URLs through this endpoint (including the -`rdm-integration` connector) must set `dataverse.api.signing-secret`. See the +- The endpoints that issue a signed URL on request - `/api/admin/requestSignedUrl` and the + guestbook-response file download (`POST /api/access/datafile/{id}`) - return an error instead of + issuing a weakly-signed URL. +- Signed callbacks and links built internally (external tool launches, Globus transfers, the + permission-history CSV links) are sent unsigned, with a warning logged, rather than weakly signed. + +Remote and Globus overlay stores are unaffected: they sign with their own per-store secret key, not +`dataverse.api.signing-secret`. + +**Upgrade note:** installations that rely on signed URLs - including the `rdm-integration` connector, +signed guestbook-response downloads, and external tools or Globus transfers that use signed callbacks - +must set `dataverse.api.signing-secret`. See the [Configuration Guide](https://guides.dataverse.org/en/latest/installation/config.html#dataverse-api-signing-secret). Treat the value like a password. Because the signing secret is part of the signing key, setting (or later changing) it invalidates previously issued signed URLs: any existing signed URLs that have not diff --git a/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java b/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java index ee687305584..7317aca1717 100644 --- a/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java +++ b/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java @@ -22,7 +22,6 @@ import edu.harvard.iq.dataverse.engine.command.exception.PermissionException; import edu.harvard.iq.dataverse.engine.command.impl.AssignRoleCommand; import edu.harvard.iq.dataverse.engine.command.impl.RevokeRoleCommand; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.BundleUtil; import edu.harvard.iq.dataverse.util.DateUtil; import edu.harvard.iq.dataverse.util.JsfHelper; @@ -640,9 +639,8 @@ public String getSignedUrlForRAHistoryCsv() { key = apiToken.getTokenString(); } } - key = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + key; - if(key.length() >= 36) { - return UrlSignerUtil.signUrl(fullApiPath, 10, userId, "GET", key); + if (key != null && UrlSignerUtil.isSigningSecretConfigured()) { + return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); } } catch (Exception e) { logger.log(Level.SEVERE, "Error generating signed URL for permissions history CSV: " + e.getMessage(), e); diff --git a/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java b/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java index f5cd859e7ac..49be3631603 100644 --- a/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java +++ b/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java @@ -20,7 +20,6 @@ import edu.harvard.iq.dataverse.engine.command.impl.CreateRoleCommand; import edu.harvard.iq.dataverse.engine.command.impl.RevokeRoleCommand; import edu.harvard.iq.dataverse.engine.command.impl.UpdateDataverseDefaultContributorRoleCommand; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.BundleUtil; import edu.harvard.iq.dataverse.util.JsfHelper; import static edu.harvard.iq.dataverse.util.JsfHelper.JH; @@ -736,9 +735,8 @@ public String getSignedUrlForRAHistoryCsv() { key = apiToken.getTokenString(); } } - key = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + key; - if(key.length() >= 36) { - return UrlSignerUtil.signUrl(fullApiPath, 10, userId, "GET", key); + if (key != null && UrlSignerUtil.isSigningSecretConfigured()) { + return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); } } catch (Exception e) { logger.log(Level.SEVERE, "Error generating signed URL for permissions history CSV: " + e.getMessage(), e); diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index a2d7d3ed525..84c44ababd2 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -29,7 +29,6 @@ import edu.harvard.iq.dataverse.makedatacount.MakeDataCountLoggingServiceBean; import edu.harvard.iq.dataverse.makedatacount.MakeDataCountLoggingServiceBean.MakeDataCountEntry; import edu.harvard.iq.dataverse.mydata.Pager; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.settings.SettingsServiceBean; import edu.harvard.iq.dataverse.util.*; import edu.harvard.iq.dataverse.util.json.JsonParseException; @@ -529,6 +528,12 @@ private Map getDatafilesMap(DataverseRequest req, String fileIds } private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, User user, String id, String gbrids) { + // Require a signing secret: without it the key is only the user's API token (or, for a guest, + // a guessable value derived from the URL), which is too weak. Mirrors Admin.getSignedUrl. + if (!UrlSignerUtil.isSigningSecretConfigured()) { + return error(INTERNAL_SERVER_ERROR, + "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); + } // Create the signed URL String userIdentifier = null; String key = null; @@ -564,8 +569,7 @@ private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, U String baseUrlEncoded = builder.build().toString(); String baseUrl = URLDecoder.decode(baseUrlEncoded, StandardCharsets.UTF_8); baseUrl = baseUrl.replace(":persistentId", id); - key = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + key; - String signedUrl = UrlSignerUtil.signUrl(baseUrl, GUESTBOOK_RESPONSE_SIGNEDURL_TIMEOUT_MINUTES, userIdentifier, "GET", key); + String signedUrl = UrlSignerUtil.signUrlWithApiKey(baseUrl, GUESTBOOK_RESPONSE_SIGNEDURL_TIMEOUT_MINUTES, userIdentifier, "GET", key); return ok(Json.createObjectBuilder().add(URLTokenUtil.SIGNED_URL, signedUrl)); } diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index e1ff9bb54fd..feb87ba32ee 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -17,7 +17,6 @@ import edu.harvard.iq.dataverse.DvObjectServiceBean; import edu.harvard.iq.dataverse.FileMetadata; import edu.harvard.iq.dataverse.api.auth.AuthRequired; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.settings.SettingsValidationException; import edu.harvard.iq.dataverse.util.StringUtil; import edu.harvard.iq.dataverse.util.cache.CacheFactoryBean; @@ -2455,7 +2454,7 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur } // Require a signing secret: without it the key is only the user's API token, which is too weak. - if (JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("").isEmpty()) { + if (!UrlSignerUtil.isSigningSecretConfigured()) { return error(Response.Status.INTERNAL_SERVER_ERROR, "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); } @@ -2481,14 +2480,13 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur if (key == null) { return error(Response.Status.CONFLICT, "Do not have a valid user with apiToken"); } - key = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + key; } - + String baseUrl = urlInfo.getString("url"); int timeout = urlInfo.getInt(URLTokenUtil.TIMEOUT, 10); String method = urlInfo.getString(URLTokenUtil.HTTP_METHOD, "GET"); - - String signedUrl = UrlSignerUtil.signUrl(baseUrl, timeout, userId, method, key); + + String signedUrl = UrlSignerUtil.signUrlWithApiKey(baseUrl, timeout, userId, method, key); return ok(Json.createObjectBuilder().add(URLTokenUtil.SIGNED_URL, signedUrl)); } diff --git a/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java b/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java index e7ae451cacf..4d0a3d49e74 100644 --- a/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java +++ b/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java @@ -4,7 +4,6 @@ import edu.harvard.iq.dataverse.Dataset; import edu.harvard.iq.dataverse.FileMetadata; import edu.harvard.iq.dataverse.authorization.users.ApiToken; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.SystemConfig; import edu.harvard.iq.dataverse.util.URLTokenUtil; @@ -111,8 +110,12 @@ public String handleRequest(boolean preview) { + externalTool.getId(); } if (apiToken != null) { - callback = UrlSignerUtil.signUrl(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(), HttpMethod.GET, - JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + apiToken.getTokenString()); + if (UrlSignerUtil.isSigningSecretConfigured()) { + callback = UrlSignerUtil.signUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(), + HttpMethod.GET, apiToken.getTokenString()); + } else { + logger.warning("Cannot sign external tool callback: no signing secret configured (dataverse.api.signing-secret). Sending an unsigned callback."); + } } paramsString= "?callback=" + Base64.getEncoder().encodeToString(StringUtils.getBytesUtf8(callback)); if (getLocaleCode() != null) { diff --git a/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java index 789e0883a7c..9d43570a9d6 100644 --- a/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java @@ -776,13 +776,14 @@ public String getGlobusAppUrlForDataset(Dataset d, boolean upload, ListStores that sign with their own per-store secret (the remote and Globus overlay stores) are + * the exception and must keep calling {@link #signUrl} directly with that secret. + * + * @throws IllegalStateException if no signing secret is configured - callers should normally + * guard with {@link #isSigningSecretConfigured()} first + */ + public static String signUrlWithApiKey(String baseUrl, Integer timeout, String user, String method, String apiKey) { + String secret = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse(""); + if (secret.isEmpty()) { + throw new IllegalStateException( + "Cannot sign a URL: no signing secret is configured. Please set the dataverse.api.signing-secret JVM option."); + } + return signUrl(baseUrl, timeout, user, method, secret + apiKey); + } + /** * Removes the reserved signing parameters from the query, preserving the exact bytes of the path * and of every other parameter (unlike URIBuilder, which would re-encode and break the MAC). diff --git a/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java b/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java index 639a7c542c4..6ed90780554 100644 --- a/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java @@ -213,6 +213,7 @@ public void testGetToolUrlWithOptionalQueryParameters() { @Test @JvmSetting(key = JvmSettings.SITE_URL, value = "https://librascholar.org") + @JvmSetting(key = JvmSettings.API_SIGNING_SECRET, value = "test-only-signing-secret") public void testGetToolUrlWithAllowedApiCalls() { System.out.println("allowedApiCalls test"); Dataset ds = new Dataset(); From 570615726332c01e9b03a946fbe37d3413fb85a7 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Mon, 6 Jul 2026 16:10:28 +0200 Subject: [PATCH 10/11] Address review findings: earlier signing-secret gate, byte-exact stripping, docs and tests #12435 - Gate the four signed-URL POST /api/access endpoints before any guestbook-response or MakeDataCount side effects, via requireSigningSecretForSignedUrl(); keep a silent backstop in returnSignedUrl for the callers that discard its response. Generic error message for these guest-reachable endpoints, actionable detail in the server log. - stripReservedParameters: preserve empty query segments and fragments byte-for-byte (split("&", -1), fragment split off before query surgery). - Log a warning and hide the permissions-history CSV link when no signing secret is set. - Tests: unsigned-fallback path, signUrlWithApiKey/isSigningSecretConfigured, strip edge cases. - Docs: signing-secret now documented as required in native-api.rst; fix env var name DATAVERSE_API_SIGNING_SECRET in config.rst; conditional-signing notes in external-tools.rst and globus-api.rst; release note lists all gated endpoints and the real error message. --- .../fix-url-signing-special-characters.md | 16 ++-- .../source/api/external-tools.rst | 4 + doc/sphinx-guides/source/api/native-api.rst | 5 +- .../source/developers/globus-api.rst | 3 +- .../source/installation/config.rst | 8 +- .../dataverse/ManageFilePermissionsPage.java | 7 +- .../iq/dataverse/ManagePermissionsPage.java | 7 +- .../edu/harvard/iq/dataverse/api/Access.java | 42 ++++++++- .../iq/dataverse/util/UrlSignerUtil.java | 32 +++++-- .../webapp/permissions-manage-files.xhtml | 1 + src/main/webapp/permissions-manage.xhtml | 5 +- .../ExternalToolHandlerTest.java | 24 +++++ .../iq/dataverse/util/UrlSignerUtilTest.java | 90 +++++++++++++++++++ 13 files changed, 214 insertions(+), 30 deletions(-) diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index d7a2853dfa2..ca29fb2badb 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -7,7 +7,7 @@ for URLs whose query contained special characters — most notably persistent ID non-ASCII characters. In 6.10 the signing step began re-encoding/normalizing the URL (for example percent-encoding `:` and `/`) before computing the signature, while the request is validated against the URL the caller actually presents back. The re-encoded signature no longer matched, so validation -failed with authentication / "signature does not match" errors. +failed with 401 "Bad signed URL" authentication errors. Signing no longer re-encodes the URL: it is signed exactly as provided, with only the reserved signing parameters (`until`, `user`, `method`, `token`, `key`, `signed`) stripped out; the rest of @@ -27,11 +27,15 @@ signed using only the user's API token (or, for a guest, a value derived from th is too weak to be a signing key. A non-empty `dataverse.api.signing-secret` is now required wherever URLs are signed with a key based on a user's API token: -- The endpoints that issue a signed URL on request - `/api/admin/requestSignedUrl` and the - guestbook-response file download (`POST /api/access/datafile/{id}`) - return an error instead of - issuing a weakly-signed URL. -- Signed callbacks and links built internally (external tool launches, Globus transfers, the - permission-history CSV links) are sent unsigned, with a warning logged, rather than weakly signed. +- The endpoints that issue a signed URL on request - `/api/admin/requestSignedUrl` and the `POST` + guestbook-response download endpoints under `/api/access` (`datafile/{id}`, `datafiles/{ids}`, + `dataset/{id}` and `dataset/{id}/versions/{versionId}`) - return an error instead of issuing a + weakly-signed URL. +- Signed callbacks built internally (external tool launches, Globus transfers) are sent unsigned, + with a warning logged, rather than weakly signed; such unsigned URLs cannot be used to access + draft datasets or restricted files. +- The permissions-history CSV download links on the permission management pages are not offered + (a warning is logged). Remote and Globus overlay stores are unaffected: they sign with their own per-store secret key, not `dataverse.api.signing-secret`. diff --git a/doc/sphinx-guides/source/api/external-tools.rst b/doc/sphinx-guides/source/api/external-tools.rst index c583c9516cc..5242b87a9c4 100644 --- a/doc/sphinx-guides/source/api/external-tools.rst +++ b/doc/sphinx-guides/source/api/external-tools.rst @@ -178,6 +178,10 @@ The signed URL mechanism is more secure than exposing API tokens and therefore r - For tools invoked via a GET call, Dataverse will include a callback query parameter with a Base64 encoded value. The decoded value is a signed URL that can be called to retrieve a JSON response containing all of the queryParameters and allowedApiCalls specified in the manfiest. - For tools invoked via POST, Dataverse will send a JSON body including the requested queryParameters and allowedApiCalls. Dataverse expects the response to the POST to indicate a redirect which Dataverse will use to open the tool. +.. note:: + + **For Dataverse site administrators:** Signing these URLs requires a non-empty :ref:`dataverse.api.signing-secret` to be configured on the Dataverse installation. Without it, the callback and ``allowedApiCalls`` URLs are sent unsigned (a warning is logged), so tools cannot use them to access draft datasets or restricted files. + .. note:: **For Dataverse site administrators:** When Dataverse is behind a proxy, signed URLs may not work correctly due to protocol mismatches (HTTP vs HTTPS). Please refer to the :ref:`signed-urls-forwarded-proto-header` section to ensure signed URLs work properly in proxy environments. diff --git a/doc/sphinx-guides/source/api/native-api.rst b/doc/sphinx-guides/source/api/native-api.rst index eaff1c77057..97604b1618e 100644 --- a/doc/sphinx-guides/source/api/native-api.rst +++ b/doc/sphinx-guides/source/api/native-api.rst @@ -9094,8 +9094,9 @@ A curl example using allowing access to a dataset's metadata curl -H "X-Dataverse-key:$API_KEY" -H 'Content-Type:application/json' -d "$JSON" "$SERVER_URL/api/admin/requestSignedUrl" -Please see :ref:`dataverse.api.signing-secret` for the configuration option to add a shared secret, enabling extra -security. +Note that a non-empty :ref:`dataverse.api.signing-secret` must be configured on the server for this endpoint to work: +the key used to sign the URL is created from the user's API token plus the signing-secret, and without the secret the +endpoint returns an error instead of issuing a weakly-signed URL. .. _send-feedback-admin: diff --git a/doc/sphinx-guides/source/developers/globus-api.rst b/doc/sphinx-guides/source/developers/globus-api.rst index eb0eb465315..36f445c3cfd 100644 --- a/doc/sphinx-guides/source/developers/globus-api.rst +++ b/doc/sphinx-guides/source/developers/globus-api.rst @@ -78,6 +78,7 @@ Note that while Dataverse will not add files that violate the size or quota rule they intend to transfer before submitting a transfer request to Globus. The getDatasetMetadata and getFileListing URLs are just signed versions of the standard Dataset metadata and file listing API calls. The other two are Globus specific. +Note that these URLs are only signed when a non-empty :ref:`dataverse.api.signing-secret` is configured on the Dataverse installation; without it, they are sent unsigned (a warning is logged) and cannot be used to access draft datasets or restricted files. If called for a dataset using a store that is configured with a remote Globus endpoint(s), the return response is similar but the response includes a the "managed" parameter will be false, the "endpoint" parameter is replaced with a JSON array of "referenceEndpointsWithPaths" and the @@ -112,7 +113,7 @@ Once the user identifies which files are to be added, the requestGlobusTransferP curl -H "X-Dataverse-key:$API_TOKEN" -H "Content-type:application/json" -X POST -d "$JSON_DATA" "$SERVER_URL/api/datasets/:persistentId/requestGlobusUploadPaths?persistentId=$PERSISTENT_IDENTIFIER" -Note that when using the dataverse-globus app or the return from the previous call, the URL for this call will be signed and no API_TOKEN is needed. +Note that when using the dataverse-globus app or the return from the previous call, the URL for this call will be signed (provided a non-empty :ref:`dataverse.api.signing-secret` is configured, as noted above) and no API_TOKEN is needed. In the managed case, the JSON body sent must include the id of the Globus user that will perform the transfer and the number of files that will be transferred: diff --git a/doc/sphinx-guides/source/installation/config.rst b/doc/sphinx-guides/source/installation/config.rst index 1dc2ad94f1c..7a8c135d4dd 100644 --- a/doc/sphinx-guides/source/installation/config.rst +++ b/doc/sphinx-guides/source/installation/config.rst @@ -3350,15 +3350,17 @@ are time limited and only allow the action of the API call in the URL. See :ref: The key used to sign a URL is created from the API token of the creating user plus a signing-secret provided by an administrator. **A non-empty signing-secret is required to request signed URLs through the API.** If it is not configured, the -``/api/admin/requestSignedUrl`` endpoint (see :ref:`api-native-signed-url`) returns an error instead of issuing a -weakly-signed URL. (The setting otherwise defaults to an empty string.) A non-empty signing-secret makes it impossible for +``/api/admin/requestSignedUrl`` endpoint (see :ref:`api-native-signed-url`) and the ``POST`` guestbook-response download +endpoints under ``/api/access`` return an error instead of issuing a weakly-signed URL, and internally generated links +(external tool callbacks, Globus transfers, the permissions-history CSV downloads) are sent unsigned or omitted, with a +warning logged. (The setting otherwise defaults to an empty string.) A non-empty signing-secret makes it impossible for someone who only knows an API token to forge signed URLs, and provides extra security by making the overall signing key longer. **WARNING**: *Since the signing-secret is sensitive, you should treat it like a password.* *See* :ref:`secure-password-storage` *to learn about ways to safeguard it.* -Can also be set via any `supported MicroProfile Config API source`_, e.g. the environment variable ``DATAVERSE_API_SIGNATURE_SECRET`` (although you shouldn't use environment variables for passwords) . +Can also be set via any `supported MicroProfile Config API source`_, e.g. the environment variable ``DATAVERSE_API_SIGNING_SECRET`` (although you shouldn't use environment variables for passwords) . .. _dataverse.api.allow-incomplete-metadata: diff --git a/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java b/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java index 7317aca1717..4e67919959f 100644 --- a/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java +++ b/src/main/java/edu/harvard/iq/dataverse/ManageFilePermissionsPage.java @@ -639,8 +639,11 @@ public String getSignedUrlForRAHistoryCsv() { key = apiToken.getTokenString(); } } - if (key != null && UrlSignerUtil.isSigningSecretConfigured()) { - return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); + if (key != null) { + if (UrlSignerUtil.isSigningSecretConfigured()) { + return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); + } + logger.warning("Cannot sign the permissions-history CSV link: no signing secret configured (dataverse.api.signing-secret). The download link will not be shown."); } } catch (Exception e) { logger.log(Level.SEVERE, "Error generating signed URL for permissions history CSV: " + e.getMessage(), e); diff --git a/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java b/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java index 49be3631603..778e28d90cf 100644 --- a/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java +++ b/src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java @@ -735,8 +735,11 @@ public String getSignedUrlForRAHistoryCsv() { key = apiToken.getTokenString(); } } - if (key != null && UrlSignerUtil.isSigningSecretConfigured()) { - return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); + if (key != null) { + if (UrlSignerUtil.isSigningSecretConfigured()) { + return UrlSignerUtil.signUrlWithApiKey(fullApiPath, 10, userId, "GET", key); + } + logger.warning("Cannot sign the permissions-history CSV link: no signing secret configured (dataverse.api.signing-secret). The download link will not be shown."); } } catch (Exception e) { logger.log(Level.SEVERE, "Error generating signed URL for permissions history CSV: " + e.getMessage(), e); diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index 84c44ababd2..dd2e39ba990 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -408,6 +408,10 @@ public Response datafile(@Context ContainerRequestContext crc, @PathParam("fileI public Response datafileWithGuestbookResponse(@Context ContainerRequestContext crc, @PathParam("fileId") String fileId, @QueryParam("gbrecs") boolean gbrecs, @Context UriInfo uriInfo, @Context HttpHeaders headers, @Context HttpServletResponse response, String jsonBody) { + Response requireSecretError = requireSigningSecretForSignedUrl(); + if (requireSecretError != null) { + return requireSecretError; + } DataverseRequest req = createDataverseRequest(getRequestUser(crc)); fileId = normalizeFileId(fileId, req); return processDatafileWithGuestbookResponse(crc, req, headers, fileId, uriInfo, gbrecs, jsonBody); @@ -442,6 +446,22 @@ private List getGuestbookIdFromDatafile(DataFile df) { return df != null && df.getOwner() != null && df.getOwner().getGuestbook() != null ? List.of(df.getOwner().getGuestbook().getId().toString()) : List.of(); } + // Gate for the endpoints whose outcome is a signed download URL. It must run before + // processDatafileWithGuestbookResponse so no guestbook-response or MakeDataCount records are + // persisted for a download URL that is never issued - but it must NOT sit inside that method, + // which is also called purely for its guestbook side effects (with the response discarded) by + // endpoints that never issue signed URLs. Without the secret the key would be only the user's + // API token (or, for a guest, a guessable value derived from the URL), which is too weak. The + // response is reachable by unauthenticated users, so the message stays generic and the + // actionable detail goes to the server log. Mirrors Admin.getSignedUrl. + private Response requireSigningSecretForSignedUrl() { + if (UrlSignerUtil.isSigningSecretConfigured()) { + return null; + } + logger.warning("Cannot issue a signed download URL: no signing secret configured. Please set the dataverse.api.signing-secret JVM option."); + return error(INTERNAL_SERVER_ERROR, "Signed URLs are not available on this server."); + } + // Process the guestbook response from JSON and return a signedUrl to the matching GET call private Response processDatafileWithGuestbookResponse(ContainerRequestContext crc, DataverseRequest req, HttpHeaders headers, String fileIds, UriInfo uriInfo, boolean gbrecs, String jsonBody) { @@ -528,11 +548,13 @@ private Map getDatafilesMap(DataverseRequest req, String fileIds } private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, User user, String id, String gbrids) { - // Require a signing secret: without it the key is only the user's API token (or, for a guest, - // a guessable value derived from the URL), which is too weak. Mirrors Admin.getSignedUrl. + // The signed-URL endpoints already check this before any guestbook side effects (see + // requireSigningSecretForSignedUrl). This check protects the callers that invoke + // processDatafileWithGuestbookResponse only for its guestbook side effects and discard this + // response: on a server without a signing secret they must not trip the IllegalStateException + // in signUrlWithApiKey below. No warning is logged here - for those callers this is routine. if (!UrlSignerUtil.isSigningSecretConfigured()) { - return error(INTERNAL_SERVER_ERROR, - "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); + return error(INTERNAL_SERVER_ERROR, "Signed URLs are not available on this server."); } // Create the signed URL String userIdentifier = null; @@ -870,6 +892,10 @@ public Response downloadAllFromLatest(@Context ContainerRequestContext crc, @Pat @Path("dataset/{id}") @Produces({"application/zip"}) public Response downloadAllFromLatestWithGuestbookResponse(@Context ContainerRequestContext crc, @PathParam("id") String datasetIdOrPersistentId, @QueryParam("gbrecs") boolean gbrecs, @Context UriInfo uriInfo, @Context HttpHeaders headers, @Context HttpServletResponse response, String jsonBody) throws WebApplicationException { + Response requireSecretError = requireSigningSecretForSignedUrl(); + if (requireSecretError != null) { + return requireSecretError; + } try { User user = getRequestUser(crc); DataverseRequest req = createDataverseRequest(user); @@ -932,6 +958,10 @@ public Response downloadAllFromVersion(@Context ContainerRequestContext crc, @Pa public Response downloadAllFromVersionWithGuestbookResponse(@Context ContainerRequestContext crc, @PathParam("id") String datasetIdOrPersistentId, @PathParam("versionId") String versionId, @QueryParam("gbrecs") boolean gbrecs, @QueryParam("key") String apiTokenParam, String jsonBody, @Context UriInfo uriInfo, @Context HttpHeaders headers, @Context HttpServletResponse response) throws WebApplicationException { + Response requireSecretError = requireSigningSecretForSignedUrl(); + if (requireSecretError != null) { + return requireSecretError; + } try { DataverseRequest req = createDataverseRequest(getRequestUser(crc)); DatasetVersion dsv = getDatasetVersionFromVersion(crc, datasetIdOrPersistentId, versionId); @@ -1016,6 +1046,10 @@ public Response datafiles(@Context ContainerRequestContext crc, @PathParam("file public Response datafilesWithGuestbookResponse(@Context ContainerRequestContext crc, @PathParam("fileIds") String fileIds, @QueryParam("gbrecs") boolean gbrecs, @Context UriInfo uriInfo, @Context HttpHeaders headers, @Context HttpServletResponse response, String jsonBody) throws WebApplicationException { + Response requireSecretError = requireSigningSecretForSignedUrl(); + if (requireSecretError != null) { + return requireSecretError; + } DataverseRequest req = createDataverseRequest(getRequestUser(crc)); return processDatafileWithGuestbookResponse(crc, req, headers, fileIds, uriInfo, gbrecs, jsonBody); } diff --git a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java index c44ef0ce729..a77d09fd078 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/UrlSignerUtil.java @@ -111,29 +111,45 @@ public static String signUrlWithApiKey(String baseUrl, Integer timeout, String u } /** - * Removes the reserved signing parameters from the query, preserving the exact bytes of the path - * and of every other parameter (unlike URIBuilder, which would re-encode and break the MAC). + * Removes the reserved signing parameters from the query, preserving the exact bytes of the path, + * of every other parameter - including empty segments such as {@code ?&a=b} or {@code a=b&&} - and + * of any fragment (unlike URIBuilder, which would re-encode and break the MAC). Note that while + * fragments are preserved byte-for-byte, a fragment is never sent to the server, so a + * fragment-bearing URL cannot produce a signed URL that validates - as was the case before 6.10. */ static String stripReservedParameters(String baseUrl) { - int queryStart = baseUrl.indexOf('?'); + // Split off the fragment first: everything from the first '#' on is the fragment (a '?' + // inside it is not a query) and survives the query surgery byte-for-byte, even when the + // fragment is attached to a reserved parameter that gets stripped. + String fragment = ""; + String prefix = baseUrl; + int fragmentStart = baseUrl.indexOf('#'); + if (fragmentStart >= 0) { + fragment = baseUrl.substring(fragmentStart); + prefix = baseUrl.substring(0, fragmentStart); + } + int queryStart = prefix.indexOf('?'); if (queryStart < 0) { return baseUrl; } - String path = baseUrl.substring(0, queryStart); - String query = baseUrl.substring(queryStart + 1); + String path = prefix.substring(0, queryStart); + String query = prefix.substring(queryStart + 1); StringBuilder kept = new StringBuilder(); - for (String pair : query.split("&")) { + boolean anyKept = false; + // limit -1 so empty segments (leading '&', '&&', trailing '&') are preserved, not collapsed + for (String pair : query.split("&", -1)) { int equals = pair.indexOf('='); String name = (equals < 0) ? pair : pair.substring(0, equals); if (reservedParameters.contains(name)) { continue; } - if (kept.length() > 0) { + if (anyKept) { kept.append('&'); } kept.append(pair); + anyKept = true; } - return kept.length() > 0 ? path + "?" + kept : path; + return anyKept ? path + "?" + kept + fragment : path + fragment; } /** diff --git a/src/main/webapp/permissions-manage-files.xhtml b/src/main/webapp/permissions-manage-files.xhtml index 045376c14c4..e37a54ef862 100644 --- a/src/main/webapp/permissions-manage-files.xhtml +++ b/src/main/webapp/permissions-manage-files.xhtml @@ -234,6 +234,7 @@
#{bundle['dataverse.permissions.history.download']} diff --git a/src/main/webapp/permissions-manage.xhtml b/src/main/webapp/permissions-manage.xhtml index 819c6c59fcc..41ee8d02a3f 100644 --- a/src/main/webapp/permissions-manage.xhtml +++ b/src/main/webapp/permissions-manage.xhtml @@ -209,8 +209,9 @@
- #{bundle['dataverse.permissions.history.download']} diff --git a/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java b/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java index 6ed90780554..6ccb5a33613 100644 --- a/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandlerTest.java @@ -242,6 +242,30 @@ public void testGetToolUrlWithAllowedApiCalls() { System.out.println(JsonUtil.prettyPrint(jo)); } + @Test + @JvmSetting(key = JvmSettings.SITE_URL, value = "https://librascholar.org") + public void testGetToolUrlWithAllowedApiCallsNoSigningSecret() { + // Without dataverse.api.signing-secret configured, the URL must be sent unsigned (no signing + // parameters at all) instead of weakly signed - and no IllegalStateException may escape. + Dataset ds = new Dataset(); + ds.setId(1L); + ApiToken at = new ApiToken(); + AuthenticatedUser au = new AuthenticatedUser(); + au.setUserIdentifier("dataverseAdmin"); + at.setAuthenticatedUser(au); + at.setTokenString("1234"); + ExternalTool et = ExternalToolServiceBeanTest.getAllowedApiCallsTool(); + URLTokenUtil externalToolHandler = new ExternalToolHandler(et, ds, at, null); + JsonObject jo = externalToolHandler + .createPostBody(externalToolHandler.getParams(JsonUtil.getJsonObject(et.getToolParameters())), JsonUtil.getJsonArray(et.getAllowedApiCalls())).build(); + String signedUrl = jo.getJsonArray("signedUrls").getJsonObject(0).getString("signedUrl"); + assertEquals("https://librascholar.org/api/v1/datasets/1", signedUrl); + assertFalse(signedUrl.contains("until=")); + assertFalse(signedUrl.contains("user=")); + assertFalse(signedUrl.contains("method=")); + assertFalse(signedUrl.contains("token=")); + } + @Test @JvmSetting(key = JvmSettings.SITE_URL, value = "https://librascholar.org") public void testDatasetConfigureTool() { diff --git a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java index 94b76a9678b..b59963b00b6 100644 --- a/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/util/UrlSignerUtilTest.java @@ -1,5 +1,8 @@ package edu.harvard.iq.dataverse.util; +import edu.harvard.iq.dataverse.settings.JvmSettings; +import edu.harvard.iq.dataverse.util.testing.JvmSetting; +import edu.harvard.iq.dataverse.util.testing.LocalJvmSettings; import jakarta.ws.rs.core.MultivaluedHashMap; import jakarta.ws.rs.core.MultivaluedMap; import org.junit.jupiter.api.Test; @@ -10,8 +13,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +@LocalJvmSettings public class UrlSignerUtilTest { @Test @@ -151,4 +156,89 @@ public void testSignedUrlIsByteExact() { assertFalse(UrlSignerUtil.isValidUrl(reEncoded, user, method, key), "a re-encoded variant must not validate (byte-exact contract)"); } + + @Test + public void testStripReservedParametersEdgeCases() { + // Empty query segments are part of the byte-exact contract and must survive unchanged. + assertEquals("http://x/api?&a=b", UrlSignerUtil.stripReservedParameters("http://x/api?&a=b")); + assertEquals("http://x/api?a=1&", UrlSignerUtil.stripReservedParameters("http://x/api?a=1&")); + assertEquals("http://x/api?a=1&&b=2", UrlSignerUtil.stripReservedParameters("http://x/api?a=1&&b=2")); + assertEquals("http://x/api?a=b&&", UrlSignerUtil.stripReservedParameters("http://x/api?a=b&&")); + assertEquals("http://x/api?", UrlSignerUtil.stripReservedParameters("http://x/api?")); + + // A reserved name with no '=' is still stripped. + assertEquals("http://x/api", UrlSignerUtil.stripReservedParameters("http://x/api?until")); + assertEquals("http://x/api?a=1", UrlSignerUtil.stripReservedParameters("http://x/api?until&a=1")); + + // Fragments survive byte-for-byte, even when attached to a stripped reserved parameter. + assertEquals("http://x/api?a=1#frag", UrlSignerUtil.stripReservedParameters("http://x/api?a=1#frag")); + assertEquals("http://x/api?a=1#frag", UrlSignerUtil.stripReservedParameters("http://x/api?a=1&token=y#frag")); + assertEquals("http://x/api#frag", UrlSignerUtil.stripReservedParameters("http://x/api?token=y#frag")); + + // A '?' inside the fragment is not a query: nothing to strip, returned unchanged. + assertEquals("http://x/api#frag?until=1", UrlSignerUtil.stripReservedParameters("http://x/api#frag?until=1")); + + // Non-reserved names that merely contain a reserved name are kept. + assertEquals("http://x/api?tokens=1&xtoken=2&user2=3", + UrlSignerUtil.stripReservedParameters("http://x/api?tokens=1&xtoken=2&user2=3")); + } + + @Test + public void testSignAndValidateEmptyQuerySegments() { + // Degenerate-but-legal query shapes must round-trip byte-exactly through sign + validate, + // so suffix-reconstructing clients (signed.substring(base.length())) keep working. + final String user = "Alice"; + final String method = "GET"; + final String key = "abracadabara open sesame"; + String[] baseUrls = new String[] { + "http://localhost:8080/api/v1/x?&a=b", + "http://localhost:8080/api/v1/x?a=1&", + "http://localhost:8080/api/v1/x?a=1&&b=2", + }; + for (String baseUrl : baseUrls) { + String signedUrl = UrlSignerUtil.signUrl(baseUrl, 1000, user, method, key); + assertTrue(signedUrl.startsWith(baseUrl + "&"), + "base URL must be preserved byte-for-byte: " + signedUrl); + assertTrue(UrlSignerUtil.isValidUrl(signedUrl, user, method, key), + "signed URL should validate when used verbatim: " + signedUrl); + } + } + + @Test + public void testIsSigningSecretConfiguredWithoutSecret() { + assertFalse(UrlSignerUtil.isSigningSecretConfigured()); + } + + @Test + @JvmSetting(key = JvmSettings.API_SIGNING_SECRET, value = "test-only-signing-secret") + public void testIsSigningSecretConfiguredWithSecret() { + assertTrue(UrlSignerUtil.isSigningSecretConfigured()); + } + + @Test + public void testSignUrlWithApiKeyRequiresSecret() { + // Without a signing secret the API-token-based signing entry point must refuse to produce + // a weakly-keyed URL. + assertThrows(IllegalStateException.class, + () -> UrlSignerUtil.signUrlWithApiKey("http://localhost:8080/api/test1", 1000, "Alice", "GET", "some-api-token")); + } + + @Test + @JvmSetting(key = JvmSettings.API_SIGNING_SECRET, value = "test-only-signing-secret") + public void testSignUrlWithApiKeySignsWithSecretPrependedToApiKey() { + final String baseUrl = "http://localhost:8080/api/v1/datasets/:persistentId?persistentId=doi:10.5072/FK2/ABC123"; + final String user = "Alice"; + final String method = "GET"; + final String apiKey = "some-api-token"; + + String signedUrl = UrlSignerUtil.signUrlWithApiKey(baseUrl, 1000, user, method, apiKey); + + // SignedUrlAuthMechanism reconstructs the key as + ; the + // signature must validate against exactly that combination and nothing weaker. + assertTrue(UrlSignerUtil.isValidUrl(signedUrl, user, method, "test-only-signing-secret" + apiKey)); + assertFalse(UrlSignerUtil.isValidUrl(signedUrl, user, method, apiKey), + "the API token alone must not validate a URL signed with the secret"); + assertFalse(UrlSignerUtil.isValidUrl(signedUrl, user, method, "test-only-signing-secret"), + "the secret alone must not validate a URL signed with secret+token"); + } } From ddcf8ffd9784368b0e3676de96252697f61b2e94 Mon Sep 17 00:00:00 2001 From: ErykKul Date: Mon, 6 Jul 2026 17:18:19 +0200 Subject: [PATCH 11/11] URL signing secret: extra polish and clarificatoin --- conf/keycloak/docker-compose-dev.yml | 1 + .../fix-url-signing-special-characters.md | 16 +++++-- doc/sphinx-guides/source/api/native-api.rst | 2 +- .../source/developers/globus-api.rst | 2 +- docker/compose/demo/compose.yml | 3 ++ .../edu/harvard/iq/dataverse/api/Access.java | 8 +++- .../edu/harvard/iq/dataverse/api/Admin.java | 47 +++++++++++-------- .../api/auth/SignedUrlAuthMechanism.java | 16 ++++++- .../externaltools/ExternalToolHandler.java | 8 +--- .../dataverse/globus/GlobusServiceBean.java | 9 ++-- .../iq/dataverse/util/URLTokenUtil.java | 8 +--- .../iq/dataverse/util/UrlSignerUtil.java | 44 +++++++++++++++-- .../api/auth/SignedUrlAuthMechanismTest.java | 36 ++++++++++++-- .../SignedUrlContainerRequestTestFake.java | 8 ++++ .../doubles/SignedUrlUriInfoTestFake.java | 20 +++++++- 15 files changed, 174 insertions(+), 54 deletions(-) diff --git a/conf/keycloak/docker-compose-dev.yml b/conf/keycloak/docker-compose-dev.yml index 7356161ec47..0e4f14b29a7 100644 --- a/conf/keycloak/docker-compose-dev.yml +++ b/conf/keycloak/docker-compose-dev.yml @@ -31,6 +31,7 @@ services: DATAVERSE_FEATURE_API_BEARER_AUTH_USE_BUILTIN_USER_ON_ID_MATCH: "1" DATAVERSE_MAIL_SYSTEM_EMAIL: "dataverse@localhost" DATAVERSE_MAIL_MTA_HOST: "smtp" + DATAVERSE_API_SIGNING_SECRET: "dev-only-signing-secret-change-me" DATAVERSE_AUTH_OIDC_ENABLED: "1" DATAVERSE_AUTH_OIDC_CLIENT_ID: test DATAVERSE_AUTH_OIDC_CLIENT_SECRET: 94XHrfNRwXsjqTqApRrwWmhDLDHpIYV8 diff --git a/doc/release-notes/fix-url-signing-special-characters.md b/doc/release-notes/fix-url-signing-special-characters.md index ca29fb2badb..eb8f55f255f 100644 --- a/doc/release-notes/fix-url-signing-special-characters.md +++ b/doc/release-notes/fix-url-signing-special-characters.md @@ -19,6 +19,12 @@ they did before 6.10 keep working unchanged, signatures are computed the same wa regression, and URLs containing special characters validate again. No client-side changes are required. +Note that, as before 6.10, the `url` submitted to `/api/admin/requestSignedUrl` (and any URL consumed +as a signed URL) must be in its URL-decoded form: the signature is computed over the URL exactly as +provided, but the server validates it against the URL-decoded request. Passing a percent-encoded URL +(e.g. `persistentId=doi%3A10.5072%2FFK2%2FABC` instead of `persistentId=doi:10.5072/FK2/ABC`) and then +using it verbatim will therefore fail validation. + ### A signing secret is now required for signed URLs Separately from the fix above, Dataverse no longer falls back to a weak signing key when @@ -31,9 +37,13 @@ URLs are signed with a key based on a user's API token: guestbook-response download endpoints under `/api/access` (`datafile/{id}`, `datafiles/{ids}`, `dataset/{id}` and `dataset/{id}/versions/{versionId}`) - return an error instead of issuing a weakly-signed URL. -- Signed callbacks built internally (external tool launches, Globus transfers) are sent unsigned, - with a warning logged, rather than weakly signed; such unsigned URLs cannot be used to access - draft datasets or restricted files. +- External tool launches send their callback unsigned (with a warning logged) rather than weakly + signed. Such an unsigned callback only allows anonymous access to public data; it cannot be used to + access draft datasets or restricted files. +- Globus transfers are effectively disabled without a signing secret: the callbacks the + dataverse-globus app relies on require an authenticated, signed request, so Globus upload (and + download of restricted or unpublished files) fails with an authorization error. Installations that + use Globus must set `dataverse.api.signing-secret`. - The permissions-history CSV download links on the permission management pages are not offered (a warning is logged). diff --git a/doc/sphinx-guides/source/api/native-api.rst b/doc/sphinx-guides/source/api/native-api.rst index 97604b1618e..fc9887e739b 100644 --- a/doc/sphinx-guides/source/api/native-api.rst +++ b/doc/sphinx-guides/source/api/native-api.rst @@ -9079,7 +9079,7 @@ Signed URLs were developed to support External Tools but may be useful in other This API call allows a Dataverse superUser to generate a signed URL for such scenarios. The JSON input parameter required is an object with the following keys: -- ``url`` - the exact URL to sign, including api version number and all query parameters +- ``url`` - the exact URL to sign, including api version number and all query parameters. Provide it in its URL-decoded form (for example ``persistentId=doi:10.5072/FK2/ABC``, not ``persistentId=doi%3A10.5072%2FFK2%2FABC``): the signature is computed over the URL exactly as provided, but Dataverse validates it against the URL-decoded request, so a percent-encoded URL used verbatim will fail validation. - ``timeOut`` - how long in minutes the signature should be valid for, default is 10 minutes - ``httpMethod`` - which HTTP method is required, default is GET - ``user`` - the user identifier for the account associated with this signature, the default is the superuser making the call. The API call will succeed/fail based on whether the specified user has the required permissions. diff --git a/doc/sphinx-guides/source/developers/globus-api.rst b/doc/sphinx-guides/source/developers/globus-api.rst index 36f445c3cfd..18e0f7f797f 100644 --- a/doc/sphinx-guides/source/developers/globus-api.rst +++ b/doc/sphinx-guides/source/developers/globus-api.rst @@ -78,7 +78,7 @@ Note that while Dataverse will not add files that violate the size or quota rule they intend to transfer before submitting a transfer request to Globus. The getDatasetMetadata and getFileListing URLs are just signed versions of the standard Dataset metadata and file listing API calls. The other two are Globus specific. -Note that these URLs are only signed when a non-empty :ref:`dataverse.api.signing-secret` is configured on the Dataverse installation; without it, they are sent unsigned (a warning is logged) and cannot be used to access draft datasets or restricted files. +Note that these URLs are only signed when a non-empty :ref:`dataverse.api.signing-secret` is configured on the Dataverse installation. Without it the callbacks are sent unsigned (a warning is logged), and because the Globus callback endpoints (``globusUploadParameters``, ``requestGlobusUploadPaths``, ``addGlobusFiles``, and the download equivalents for restricted or unpublished files) require an authenticated, signed request, the Globus transfer flow fails with an authorization error; only anonymous download of public files can proceed. In practice, Globus transfer support requires ``dataverse.api.signing-secret`` to be set. If called for a dataset using a store that is configured with a remote Globus endpoint(s), the return response is similar but the response includes a the "managed" parameter will be false, the "endpoint" parameter is replaced with a JSON array of "referenceEndpointsWithPaths" and the diff --git a/docker/compose/demo/compose.yml b/docker/compose/demo/compose.yml index 779cf37a931..b0be4f87ea9 100644 --- a/docker/compose/demo/compose.yml +++ b/docker/compose/demo/compose.yml @@ -16,6 +16,9 @@ services: DATAVERSE_FEATURE_API_BEARER_AUTH: "1" DATAVERSE_MAIL_SYSTEM_EMAIL: "Demo Dataverse " DATAVERSE_MAIL_MTA_HOST: "smtp" + # Required for signed URLs (external tools, Globus transfers, signed guestbook downloads, + # rdm-integration). Change this to a private value; see the Configuration Guide. + DATAVERSE_API_SIGNING_SECRET: "demo-only-signing-secret-change-me" JVM_ARGS: -Ddataverse.files.storage-driver-id=file1 -Ddataverse.files.file1.type=file -Ddataverse.files.file1.label=Filesystem diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index dd2e39ba990..73e80937534 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -548,6 +548,13 @@ private Map getDatafilesMap(DataverseRequest req, String fileIds } private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, User user, String id, String gbrids) { + // Record the guestbook-response id for the side-effect-only callers + // (datafileBundleWithGuestbookResponse, postDownloadDatafiles) that discard the Response we + // return and then read this property to drive the actual download. This must happen even when + // no signing secret is configured and we bail out below, otherwise those downloads lose the + // guestbook response that processDatafileWithGuestbookResponse just saved (leading to a + // spurious "guestbookResponseMissing" 400 or a duplicate guestbook/MakeDataCount entry). + crc.setProperty("gbrids", gbrids); // The signed-URL endpoints already check this before any guestbook side effects (see // requireSigningSecretForSignedUrl). This check protects the callers that invoke // processDatafileWithGuestbookResponse only for its guestbook side effects and discard this @@ -587,7 +594,6 @@ private Response returnSignedUrl(ContainerRequestContext crc, UriInfo uriInfo, U builder.replaceQueryParam("gbrids", gbrids); } builder.replaceQueryParam("persistentId", null); // remove this as a parm and add the id to the path - crc.setProperty("gbrids", gbrids); String baseUrlEncoded = builder.build().toString(); String baseUrl = URLDecoder.decode(baseUrlEncoded, StandardCharsets.UTF_8); baseUrl = baseUrl.replace(":persistentId", id); diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index feb87ba32ee..6c1c49113a9 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -2459,30 +2459,37 @@ public Response getSignedUrl(@Context ContainerRequestContext crc, JsonObject ur "Requesting signed URLs requires a signing secret to be configured. Please set the dataverse.api.signing-secret JVM option."); } - String userId = urlInfo.getString("user"); - String key=null; - if (userId != null) { - AuthenticatedUser user = authSvc.getAuthenticatedUser(userId); - // If a user param was sent, we sign the URL for them, otherwise on behalf of - // the superuser who made this api call - if (user != null) { - ApiToken apiToken = authSvc.findApiTokenByUser(user); - if (apiToken != null && !apiToken.isExpired() && !apiToken.isDisabled()) { - key = apiToken.getTokenString(); - } - } else { - userId = superuser.getUserIdentifier(); - // We ~know this exists - the superuser just used it and it was unexpired/not - // disabled. (ToDo - if we want this to work with workflow tokens (or as a - // signed URL), we should do more checking as for the user above)) - key = authSvc.findApiTokenByUser(superuser).getTokenString(); + // "url" is required; "user" defaults to the superuser making the call (see the docs for this + // endpoint). Use the defaulted accessors: JsonObject.getString(name) throws NullPointerException + // when the key is absent. + String baseUrl = urlInfo.getString("url", null); + if (baseUrl == null) { + return error(Response.Status.BAD_REQUEST, "Required parameter 'url' is missing."); + } + String userId = urlInfo.getString("user", null); + + String key = null; + AuthenticatedUser signingUser = (userId != null) ? authSvc.getAuthenticatedUser(userId) : null; + if (signingUser != null) { + // A known user was requested: sign the URL for them. + ApiToken apiToken = authSvc.findApiTokenByUser(signingUser); + if (apiToken != null && !apiToken.isExpired() && !apiToken.isDisabled()) { + key = apiToken.getTokenString(); } - if (key == null) { - return error(Response.Status.CONFLICT, "Do not have a valid user with apiToken"); + } else { + // No user param, or an unknown one: sign on behalf of the superuser who made this API call. + userId = superuser.getUserIdentifier(); + // The superuser just authenticated, but that does not guarantee an API token exists (e.g. + // bearer-token or session auth), so null-check rather than dereference blindly. + ApiToken apiToken = authSvc.findApiTokenByUser(superuser); + if (apiToken != null) { + key = apiToken.getTokenString(); } } + if (key == null) { + return error(Response.Status.CONFLICT, "Do not have a valid user with apiToken"); + } - String baseUrl = urlInfo.getString("url"); int timeout = urlInfo.getInt(URLTokenUtil.TIMEOUT, 10); String method = urlInfo.getString(URLTokenUtil.HTTP_METHOD, "GET"); diff --git a/src/main/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanism.java b/src/main/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanism.java index d21d91a07c2..0e65d8678a3 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanism.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/auth/SignedUrlAuthMechanism.java @@ -4,7 +4,6 @@ import edu.harvard.iq.dataverse.authorization.users.*; import edu.harvard.iq.dataverse.privateurl.PrivateUrl; import edu.harvard.iq.dataverse.privateurl.PrivateUrlServiceBean; -import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.UrlSignerUtil; import jakarta.inject.Inject; @@ -52,6 +51,14 @@ private String getSignedUrlRequestParameter(ContainerRequestContext containerReq private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext containerRequestContext) { User user = null; + // Without a signing secret we never issue signed URLs (signUrlWithApiKey refuses on the sign + // side), so we must not accept them here either. Otherwise a bare API token - or, for a guest, + // the public request URL - would be enough to forge a URL whose signature validates against the + // "" + token key computed below. Reject so findUserFromRequest returns the standard 401. + if (!UrlSignerUtil.isSigningSecretConfigured()) { + logger.warning("Rejecting signed URL authentication: no signing secret configured (dataverse.api.signing-secret)."); + return null; + } // The signedUrl contains a param telling which user this is supposed to be for. // We don't trust this. So we look up that user, and get their API key, and use // that as a secret in validating the signedURL. If the signature can't be @@ -60,6 +67,11 @@ private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext container // If User is Guest we can return a generic guest user with key made from URI UriInfo uriInfo = containerRequestContext.getUriInfo(); String userId = uriInfo.getQueryParameters().getFirst(SIGNED_URL_USER); + if (userId == null) { + // A token param was present (that is why this mechanism ran) but no user param: this can + // never be a URL we signed, and dereferencing userId below would throw a NullPointerException. + return null; + } User targetUser = null; ApiToken userApiToken = null; if (userId.equalsIgnoreCase("guest")) { @@ -90,7 +102,7 @@ private User getAuthenticatedUserFromSignedUrl(ContainerRequestContext container } String requestMethod = containerRequestContext.getMethod(); - String signedUrlSigningKey = JvmSettings.API_SIGNING_SECRET.lookupOptional().orElse("") + userApiToken.getTokenString(); + String signedUrlSigningKey = UrlSignerUtil.getApiSigningKey(userApiToken.getTokenString()); boolean isSignedUrlValid = UrlSignerUtil.isValidUrl(signedUrl, userId, requestMethod, signedUrlSigningKey); if (isSignedUrlValid) { user = targetUser; diff --git a/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java b/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java index 4d0a3d49e74..ed770f1934b 100644 --- a/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java +++ b/src/main/java/edu/harvard/iq/dataverse/externaltools/ExternalToolHandler.java @@ -110,12 +110,8 @@ public String handleRequest(boolean preview) { + externalTool.getId(); } if (apiToken != null) { - if (UrlSignerUtil.isSigningSecretConfigured()) { - callback = UrlSignerUtil.signUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(), - HttpMethod.GET, apiToken.getTokenString()); - } else { - logger.warning("Cannot sign external tool callback: no signing secret configured (dataverse.api.signing-secret). Sending an unsigned callback."); - } + callback = UrlSignerUtil.trySignUrlWithApiKey(callback, 5, apiToken.getAuthenticatedUser().getUserIdentifier(), + HttpMethod.GET, apiToken.getTokenString(), "external tool callback"); } paramsString= "?callback=" + Base64.getEncoder().encodeToString(StringUtils.getBytesUtf8(callback)); if (getLocaleCode() != null) { diff --git a/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java index 9d43570a9d6..65d8e2ac08c 100644 --- a/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/globus/GlobusServiceBean.java @@ -779,11 +779,12 @@ public String getGlobusAppUrlForDataset(Dataset d, boolean upload, List the signature must no longer validate. String tampered = signedUrl.replace("FK2/ABC", "FK2/HACKED"); @@ -128,12 +141,26 @@ public void testEndToEnd_tamperedSignedUrl_userNotAuthenticated() { assertThrows(WrappedUnauthorizedAuthErrorResponse.class, () -> sut.findUserFromRequest(request)); } + @Test + public void testEndToEnd_noSigningSecret_signedUrlRejected() { + // With no signing secret configured, a URL signed with only the bare API token would still hash + // valid (key = "" + token) - but the mechanism must refuse it, otherwise a leaked/expired token + // or a guest key derived from the public URL could be used to forge a signed URL. + givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); + String base = "http://localhost:8080/api/v1/datasets/1"; + String signedUrl = UrlSignerUtil.signUrl(base, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + + ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, signedUrl); + + assertThrows(WrappedUnauthorizedAuthErrorResponse.class, () -> sut.findUserFromRequest(request)); + } + // Runs the real rdm flow: un-escape, sign, request the original (encoded) URL + signature, then the // server URL-decodes the request and checks it. Returns true iff the user authenticates. private boolean validatesEndToEndAsRdmClient(String urlAsClientBuilds) { givenUserWithSigningKey(TEST_SIGNED_URL_TOKEN); String canonical = URLDecoder.decode(urlAsClientBuilds, StandardCharsets.UTF_8); - String signed = UrlSignerUtil.signUrl(canonical, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); + String signed = UrlSignerUtil.signUrlWithApiKey(canonical, 1000, TEST_SIGNED_URL_USER_ID, "GET", TEST_SIGNED_URL_TOKEN); String requestUri = urlAsClientBuilds + signed.substring(canonical.length()); ContainerRequestContext request = new SignedUrlContainerRequestTestFake(TEST_SIGNED_URL_TOKEN, TEST_SIGNED_URL_USER_ID, requestUri); try { @@ -144,6 +171,7 @@ private boolean validatesEndToEndAsRdmClient(String urlAsClientBuilds) { } @Test + @JvmSetting(key = JvmSettings.API_SIGNING_SECRET, value = TEST_SIGNING_SECRET) public void testEndToEnd_allRdmIntegrationUrls_authenticate() { final String s = "https://demo.dataverse.org"; final String pid = "doi:10.5072/FK2/ABC"; // raw, as most rdm paths send it diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java index 6a1a440f713..c4c1360f2fc 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlContainerRequestTestFake.java @@ -17,4 +17,12 @@ public SignedUrlContainerRequestTestFake(String signedUrlToken, String signedUrl public UriInfo getUriInfo() { return uriInfo; } + + @Override + public String getMethod() { + // The signed URLs these fakes build are signed for GET; returning it (rather than the base + // class's null) makes SignedUrlAuthMechanism actually exercise the HTTP-method check in + // UrlSignerUtil.isValidUrl instead of silently skipping it. + return "GET"; + } } diff --git a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java index 5edd31509f8..4c3e5a33101 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/auth/doubles/SignedUrlUriInfoTestFake.java @@ -6,6 +6,8 @@ import jakarta.ws.rs.core.MultivaluedMap; import java.net.URI; +import java.net.URLDecoder; +import java.nio.charset.StandardCharsets; import static edu.harvard.iq.dataverse.util.UrlSignerUtil.SIGNED_URL_TOKEN; import static edu.harvard.iq.dataverse.util.UrlSignerUtil.SIGNED_URL_USER; @@ -38,12 +40,28 @@ public URI getRequestUri() { if (requestUriOverride != null) { return URI.create(requestUriOverride); } - return URI.create(UrlSignerUtil.signUrl(SIGNED_URL_BASE_URL, SIGNED_URL_TIMEOUT, signedUrlUserId, GET, signedUrlToken)); + // Sign the way the server does: with the configured signing secret prepended to the token. This + // keeps the fake consistent with SignedUrlAuthMechanism, which validates with secret + token. + return URI.create(UrlSignerUtil.signUrlWithApiKey(SIGNED_URL_BASE_URL, SIGNED_URL_TIMEOUT, signedUrlUserId, GET, signedUrlToken)); } @Override public MultivaluedMap getQueryParameters() { MultivaluedMap queryParameters = new MultivaluedHashMap<>(); + if (requestUriOverride != null) { + // Reflect the actual request URI (like Jersey would) so tests exercise real extraction of + // the user/token params from the URL under test, not fabricated values. + String query = URI.create(requestUriOverride).getRawQuery(); + if (query != null) { + for (String pair : query.split("&")) { + int equals = pair.indexOf('='); + String name = (equals < 0) ? pair : pair.substring(0, equals); + String value = (equals < 0) ? "" : URLDecoder.decode(pair.substring(equals + 1), StandardCharsets.UTF_8); + queryParameters.add(name, value); + } + } + return queryParameters; + } queryParameters.add(SIGNED_URL_TOKEN, signedUrlToken); queryParameters.add(SIGNED_URL_USER, signedUrlUserId); return queryParameters;