This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 7f1dd0b182 [#14305,#14306,#14437,#14442] fix(secret): Harden secret
binding and catalog ops (#14479)
7f1dd0b182 is described below
commit 7f1dd0b182cd42f6fa3e48d6ecfa52b8d7349c73
Author: MaSai <[email protected]>
AuthorDate: Tue Nov 3 16:15:01 2026 +0800
[#14305,#14306,#14437,#14442] fix(secret): Harden secret binding and
catalog ops (#14479)
### What changes were proposed in this pull request?
- Map unsupported metadata-object secret reads to 400
(`IllegalArgumentException` / legacy `NotSupportedException`).
- Reject create-time `secretBindings` whose plaintext is the masked
placeholder `******` (same rule as alter `setSecretBinding`).
- Reject scalar/array `SecretBindingDTO` values without quoting them,
and omit Jackson stacks from malformed-JSON 400 responses so plaintext
is not echoed.
- Add `SecretManager.toPlaintextOrMasked` for catalog load so list /
disable / drop / re-bind still work when a bound secret cannot be
resolved; isolate listing failures per catalog.
### Why are the changes needed?
Fixes #14305, #14306, #14437, #14442.
Incomplete or missing secrets currently surface as 500s, leak plaintext
in error stacks, accept the read-path mask as a real secret on create,
or brick catalog management and detailed listing when the provider no
longer has the material.
### Does this PR introduce _any_ user-facing change?
Yes.
- Unsupported object-type secret GETs return 400 instead of 500.
- Create with `plaintext: "******"` is rejected.
- Malformed `secretBindings` 400 responses no longer include Jackson
stacks that may quote secrets.
- Catalogs whose secrets cannot be resolved remain manageable (list /
disable / drop / re-bind).
### How was this patch tested?
- `./gradlew :common:test --tests
org.apache.gravitino.dto.secret.TestSecretBindingDTO`
- `./gradlew :core:test --tests
org.apache.gravitino.secret.TestSecretManager --tests
org.apache.gravitino.catalog.TestCatalogManager.testCreateRejectsMaskedPlaceholderSecretBinding
--tests
org.apache.gravitino.catalog.TestCatalogManager.testCatalogManageableWhenSecretMissing`
- `./gradlew :server:test --tests
org.apache.gravitino.server.web.rest.TestExceptionHandlers.testUnsupportedSecretObjectTypeReturnsBadRequest
--tests
org.apache.gravitino.server.web.mapper.TestJsonMappingExceptionMapper`
Made with [Cursor](https://cursor.com)
---------
Co-authored-by: Cursor <[email protected]>
---
.../gravitino/dto/secret/SecretBindingDTO.java | 46 +++++++++++++
.../gravitino/dto/secret/TestSecretBindingDTO.java | 66 +++++++++++++++++++
.../apache/gravitino/catalog/CatalogManager.java | 30 ++++++++-
.../apache/gravitino/connector/BaseCatalog.java | 4 +-
.../org/apache/gravitino/secret/SecretManager.java | 46 ++++++++++++-
.../gravitino/catalog/TestCatalogManager.java | 77 ++++++++++++++++++++++
.../apache/gravitino/secret/TestSecretManager.java | 35 ++++++++++
.../web/mapper/JsonMappingExceptionMapper.java | 4 +-
.../server/web/rest/ExceptionHandlers.java | 3 +-
.../web/rest/MetadataObjectSecretOperations.java | 3 +-
.../web/mapper/TestJsonMappingExceptionMapper.java | 5 +-
.../server/web/rest/TestExceptionHandlers.java | 14 ++++
12 files changed, 322 insertions(+), 11 deletions(-)
diff --git
a/common/src/main/java/org/apache/gravitino/dto/secret/SecretBindingDTO.java
b/common/src/main/java/org/apache/gravitino/dto/secret/SecretBindingDTO.java
index fd029fdb39..ace17440bb 100644
--- a/common/src/main/java/org/apache/gravitino/dto/secret/SecretBindingDTO.java
+++ b/common/src/main/java/org/apache/gravitino/dto/secret/SecretBindingDTO.java
@@ -19,7 +19,14 @@
package org.apache.gravitino.dto.secret;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.google.common.collect.ImmutableMap;
+import java.io.IOException;
import java.util.Map;
import javax.annotation.Nullable;
import lombok.AccessLevel;
@@ -38,6 +45,7 @@ import org.apache.gravitino.secret.SecretBinding;
@AllArgsConstructor
@Builder(setterPrefix = "with")
@ToString(exclude = "plaintext")
+@JsonDeserialize(using = SecretBindingDTO.Deserializer.class)
public class SecretBindingDTO {
@JsonProperty("provider")
@@ -105,4 +113,42 @@ public class SecretBindingDTO {
}
return dtos.build();
}
+
+ /**
+ * Rejects scalar / array {@code secretBindings} values without quoting them
in the exception
+ * message, so a mistaken bare-string binding does not echo the secret in
the HTTP 400 stack.
+ */
+ public static final class Deserializer extends
JsonDeserializer<SecretBindingDTO> {
+
+ @Override
+ public SecretBindingDTO deserialize(JsonParser p, DeserializationContext
ctxt)
+ throws IOException {
+ JsonNode node = p.getCodec().readTree(p);
+ if (node == null || node.isNull()) {
+ return null;
+ }
+ if (!node.isObject()) {
+ // Pass a null parser so Jackson does not append the input source
(which may be the
+ // secret plaintext) into the exception message / HTTP error stack.
+ throw new JsonMappingException(
+ null,
+ "secretBindings values must be JSON objects with 'provider' and
'plaintext', not a"
+ + " scalar or array");
+ }
+ if (node.has("plaintext")
+ && !node.get("plaintext").isNull()
+ && !node.get("plaintext").isTextual()) {
+ throw new JsonMappingException(null, "secretBindings.plaintext must be
a string");
+ }
+ String provider =
+ node.has("provider") && !node.get("provider").isNull()
+ ? node.get("provider").asText()
+ : null;
+ String plaintext =
+ node.has("plaintext") && !node.get("plaintext").isNull()
+ ? node.get("plaintext").asText()
+ : null;
+ return
SecretBindingDTO.builder().withProvider(provider).withPlaintext(plaintext).build();
+ }
+ }
}
diff --git
a/common/src/test/java/org/apache/gravitino/dto/secret/TestSecretBindingDTO.java
b/common/src/test/java/org/apache/gravitino/dto/secret/TestSecretBindingDTO.java
new file mode 100644
index 0000000000..d49efc13d6
--- /dev/null
+++
b/common/src/test/java/org/apache/gravitino/dto/secret/TestSecretBindingDTO.java
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.dto.secret;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.gravitino.json.JsonUtils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/** SerDe tests for {@link SecretBindingDTO}, including scalar-rejection
without plaintext echo. */
+public class TestSecretBindingDTO {
+
+ private static final ObjectMapper MAPPER = JsonUtils.objectMapper();
+
+ @Test
+ void testObjectRoundTrip() throws Exception {
+ SecretBindingDTO dto =
+
SecretBindingDTO.builder().withProvider("memory").withPlaintext("s3cr3t").build();
+ String json = MAPPER.writeValueAsString(dto);
+ SecretBindingDTO parsed = MAPPER.readValue(json, SecretBindingDTO.class);
+ Assertions.assertEquals("memory", parsed.getProvider());
+ Assertions.assertEquals("s3cr3t", parsed.getPlaintext());
+ }
+
+ @Test
+ void testScalarBindingDoesNotEchoPlaintext() {
+ String secret = "MY-SECRET-VALUE-123";
+ JsonMappingException ex =
+ Assertions.assertThrows(
+ JsonMappingException.class,
+ () -> MAPPER.readValue("\"" + secret + "\"",
SecretBindingDTO.class));
+ Assertions.assertFalse(ex.getMessage().contains(secret));
+ Assertions.assertTrue(ex.getMessage().contains("JSON objects"));
+ }
+
+ @Test
+ void testArrayPlaintextRejectedWithoutEcho() {
+ String secret = "LIST-SECRET-999";
+ JsonProcessingException ex =
+ Assertions.assertThrows(
+ JsonProcessingException.class,
+ () ->
+ MAPPER.readValue(
+ "{\"provider\":\"memory\",\"plaintext\":[\"" + secret +
"\"]}",
+ SecretBindingDTO.class));
+ Assertions.assertFalse(ex.getMessage().contains(secret));
+ }
+}
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
index d0e850b402..5bf613b92d 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -84,6 +84,7 @@ import org.apache.gravitino.connector.BaseCatalog;
import org.apache.gravitino.connector.CatalogDropAware;
import org.apache.gravitino.connector.CatalogOperations;
import org.apache.gravitino.connector.HasPropertyMetadata;
+import org.apache.gravitino.connector.HiddenPropertyMaskUtils;
import org.apache.gravitino.connector.SupportsSchemas;
import org.apache.gravitino.connector.authorization.BaseAuthorization;
import org.apache.gravitino.connector.capability.Capability;
@@ -711,7 +712,7 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
// provider is "fileset", still using "hadoop" will lead to catalog
loading issue. So
// after reading the catalog entity, we convert it to the new
fileset catalog entity.
.map(this::convertFilesetCatalogEntity)
- .map(e -> e.toCatalogInfoWithResolvedProps(getResolvedProperties(e)))
+ .map(this::toCatalogInfoForList)
.toArray(Catalog[]::new);
} catch (IOException ioe) {
LOG.error("Failed to list catalogs in metalake {}", metalakeIdent, ioe);
@@ -1940,6 +1941,30 @@ public class CatalogManager implements
CatalogDispatcher, Closeable {
}
}
+ /**
+ * Builds a catalog info for {@link #listCatalogsInfo}. A single catalog
that fails to resolve
+ * (for example after a temporary ClassLoader error) must not fail the
entire metalake listing.
+ */
+ private Catalog toCatalogInfoForList(CatalogEntity entity) {
+ try {
+ return
entity.toCatalogInfoWithResolvedProps(getResolvedProperties(entity));
+ } catch (RuntimeException e) {
+ LOG.warn(
+ "Failed to resolve catalog {} while listing; returning entity
properties with secrets"
+ + " masked",
+ entity.nameIdentifier(),
+ e);
+ Map<String, String> props =
+ entity.getProperties() == null ? new HashMap<>() : new
HashMap<>(entity.getProperties());
+ for (Map.Entry<String, String> entry : props.entrySet()) {
+ if (SecretPropertyUtils.isSecretProperty(entry.getKey(),
entry.getValue())) {
+ entry.setValue(HiddenPropertyMaskUtils.MASKED_VALUE);
+ }
+ }
+ return entity.toCatalogInfoWithResolvedProps(props);
+ }
+ }
+
private Catalog toCatalogInfo(CatalogWrapper wrapper) {
try {
return wrapper.doWithCatalog(
@@ -1972,8 +1997,9 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
// Load Catalog class instance
BaseCatalog<?> catalog = createCatalogInstance(classLoader,
entity.getProvider());
// Resolve secret URNs to plaintext for connector init only; entity
storage keeps URNs.
+ // toPlaintextOrMasked: a deleted/rotated secret must not block disable,
drop, re-bind or list.
catalog
-
.withCatalogConf(secretManager.toPlaintextProperties(entity.getProperties()))
+
.withCatalogConf(secretManager.toPlaintextOrMasked(entity.getProperties()))
.withCatalogEntity(entity)
.withSecretManager(secretManager);
catalog.initAuthorizationPluginInstance(classLoader, metalakeEntity.id());
diff --git a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
index afa2daaec0..9871e50232 100644
--- a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
+++ b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
@@ -626,9 +626,11 @@ public abstract class BaseCatalog<T extends BaseCatalog>
*/
public Map<String, String> propertiesWithCredentialProviders() {
// Entity storage keeps secret URNs; resolve to plaintext when
SecretManager is available.
+ // toPlaintextOrMasked so a deleted/rotated secret does not block catalog
load (disable, drop,
+ // list, re-bind). Unresolved secrets become the masked placeholder.
Map<String, String> props = Maps.newHashMap(entity().getProperties());
if (secretManager != null) {
- props = Maps.newHashMap(secretManager.toPlaintextProperties(props));
+ props = Maps.newHashMap(secretManager.toPlaintextOrMasked(props));
}
if
(StringUtils.isNotBlank(props.get(CredentialConstants.CREDENTIAL_PROVIDERS))) {
// Explicit credential-providers wins: do not auto-append detected
static providers (e.g.
diff --git a/core/src/main/java/org/apache/gravitino/secret/SecretManager.java
b/core/src/main/java/org/apache/gravitino/secret/SecretManager.java
index aa88fd4cf5..43dd1c0ec3 100644
--- a/core/src/main/java/org/apache/gravitino/secret/SecretManager.java
+++ b/core/src/main/java/org/apache/gravitino/secret/SecretManager.java
@@ -36,6 +36,7 @@ import java.util.Set;
import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.Config;
+import org.apache.gravitino.connector.HiddenPropertyMaskUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -515,13 +516,32 @@ public class SecretManager implements Closeable {
* Returns a copy of {@code properties} with secret URN values replaced by
plaintext.
*
* <p>Used so connectors receive plaintext conf while entity storage keeps
URN strings. Non-secret
- * entries are copied unchanged.
+ * entries are copied unchanged. Missing secrets fail with {@link
IllegalArgumentException}.
*
* @param properties entity or request properties (may be null)
* @return a new map with secret URNs replaced by plaintext; empty map when
{@code properties} is
* null
*/
public Map<String, String> toPlaintextProperties(@Nullable Map<String,
String> properties) {
+ return resolvePlaintextProperties(properties, true);
+ }
+
+ /**
+ * Like {@link #toPlaintextProperties(Map)}, but unresolved secrets become
{@link
+ * HiddenPropertyMaskUtils#MASKED_VALUE} instead of throwing.
+ *
+ * <p>Catalog load for disable / drop / re-bind / listing uses this so a
deleted or rotated secret
+ * does not block management of the catalog.
+ *
+ * @param properties entity or request properties (may be null)
+ * @return a new map with secret URNs replaced by plaintext or the masked
placeholder
+ */
+ public Map<String, String> toPlaintextOrMasked(@Nullable Map<String, String>
properties) {
+ return resolvePlaintextProperties(properties, false);
+ }
+
+ private Map<String, String> resolvePlaintextProperties(
+ @Nullable Map<String, String> properties, boolean failOnMissing) {
if (properties == null || properties.isEmpty()) {
return properties == null ? Map.of() : Map.copyOf(properties);
}
@@ -530,7 +550,19 @@ public class SecretManager implements Closeable {
String key = entry.getKey();
String value = entry.getValue();
if (SecretPropertyUtils.isSecretProperty(key, value)) {
- plaintext.put(key, readSecret(SecretUrn.parse(value)));
+ try {
+ plaintext.put(key, readSecret(SecretUrn.parse(value)));
+ } catch (IllegalArgumentException e) {
+ if (failOnMissing) {
+ throw e;
+ }
+ LOG.warn(
+ "Secret for property '{}' could not be resolved during catalog
load; "
+ + "using masked placeholder so management operations can
proceed: {}",
+ key,
+ e.getMessage());
+ plaintext.put(key, HiddenPropertyMaskUtils.MASKED_VALUE);
+ }
} else {
plaintext.put(key, value);
}
@@ -551,7 +583,15 @@ public class SecretManager implements Closeable {
SecretBinding binding = bindings.get(propertyKey);
Preconditions.checkArgument(
binding != null, "No secretBindings entry for property key \"%s\"",
propertyKey);
- secretMaterials.add(new SecretMaterial(urn, binding.plaintext()));
+ String plaintext = binding.plaintext();
+ // Same rule as alter setSecretBinding: never persist the read-path mask
as a real secret.
+ if (HiddenPropertyMaskUtils.isMaskedPlaceholder(plaintext)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Property '%s' cannot be set to the masked placeholder value
'%s'",
+ propertyKey, HiddenPropertyMaskUtils.MASKED_VALUE));
+ }
+ secretMaterials.add(new SecretMaterial(urn, plaintext));
}
return List.copyOf(secretMaterials);
}
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
index 31bdae35c1..e375280325 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -41,6 +41,7 @@ import java.nio.file.Path;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -2502,6 +2503,82 @@ public class TestCatalogManager {
}
}
+ @Test
+ void testCreateRejectsMaskedPlaceholderSecretBinding() throws Exception {
+ try (SecretManager secrets = memorySecretManager();
+ CatalogManager manager =
+ new CatalogManager(config, entityStore, new RandomIdGenerator(),
secrets)) {
+ NameIdentifier ident = NameIdentifier.of("metalake",
"secret_mask_create");
+ IllegalArgumentException ex =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ manager.createCatalog(
+ ident,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "comment",
+ catalogProps(),
+ Map.of(
+ PROPERTY_KEY4,
+ new SecretBinding("memory",
HiddenPropertyMaskUtils.MASKED_VALUE)),
+ Map.of()));
+ Assertions.assertTrue(ex.getMessage().contains("masked placeholder"));
+ Assertions.assertFalse(manager.catalogExists(ident));
+ }
+ }
+
+ @Test
+ void testCatalogManageableWhenSecretMissing() throws Exception {
+ try (SecretManager secrets = memorySecretManager();
+ CatalogManager manager =
+ new CatalogManager(config, entityStore, new RandomIdGenerator(),
secrets)) {
+ NameIdentifier broken = NameIdentifier.of("metalake", "secret_missing");
+ NameIdentifier healthy = NameIdentifier.of("metalake", "secret_healthy");
+ manager.createCatalog(
+ broken,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "comment",
+ catalogProps(),
+ Map.of(PROPERTY_KEY4, new SecretBinding("memory", "gone-soon")),
+ Map.of());
+ manager.createCatalog(healthy, Catalog.Type.RELATIONAL, provider,
"comment", catalogProps());
+
+ String urn =
+ entityStore
+ .get(broken, EntityType.CATALOG, CatalogEntity.class)
+ .getProperties()
+ .get(PROPERTY_KEY4);
+ secrets.deleteSecretsFromProperties(Map.of(PROPERTY_KEY4, urn));
+ manager.getCatalogCache().invalidate(broken);
+
+ Catalog[] listed = manager.listCatalogsInfo(broken.namespace());
+ Assertions.assertTrue(Arrays.stream(listed).anyMatch(c ->
c.name().equals(broken.name())));
+ Assertions.assertTrue(Arrays.stream(listed).anyMatch(c ->
c.name().equals(healthy.name())));
+
+ // Re-bind while still in use (alter requires catalog in use).
+ Assertions.assertDoesNotThrow(
+ () ->
+ manager.alterCatalog(
+ broken,
+ CatalogChange.setSecretBinding(
+ PROPERTY_KEY4, new SecretBinding("memory", "rebound"))));
+ String reboundUrn =
+ entityStore
+ .get(broken, EntityType.CATALOG, CatalogEntity.class)
+ .getProperties()
+ .get(PROPERTY_KEY4);
+ Assertions.assertEquals("rebound",
secrets.readSecret(SecretUrn.parse(reboundUrn)));
+
+ secrets.deleteSecretsFromProperties(Map.of(PROPERTY_KEY4, reboundUrn));
+ manager.getCatalogCache().invalidate(broken);
+ Assertions.assertDoesNotThrow(() -> manager.disableCatalog(broken));
+ Assertions.assertTrue(manager.dropCatalog(broken, true));
+ Assertions.assertTrue(manager.dropCatalog(healthy, true));
+ }
+ }
+
private static Map<String, String> catalogProps() {
return ImmutableMap.of(
"provider",
diff --git
a/core/src/test/java/org/apache/gravitino/secret/TestSecretManager.java
b/core/src/test/java/org/apache/gravitino/secret/TestSecretManager.java
index ccc1f54d0d..10430a3457 100644
--- a/core/src/test/java/org/apache/gravitino/secret/TestSecretManager.java
+++ b/core/src/test/java/org/apache/gravitino/secret/TestSecretManager.java
@@ -71,6 +71,41 @@ public class TestSecretManager {
}
}
+ @Test
+ void testToPlaintextOrMaskedWhenMissing() {
+ try (SecretManager sm = memorySecretManager()) {
+ Map<String, String> props = new HashMap<>();
+ List<SecretUrn> urns = sm.buildSecretBindingUrns("catalog", 42L,
BINDINGS);
+ SecretPropertyUtils.putSecretUrns(props, urns);
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
sm.toPlaintextProperties(props));
+ Map<String, String> masked = sm.toPlaintextOrMasked(props);
+ Assertions.assertEquals(
+ org.apache.gravitino.connector.HiddenPropertyMaskUtils.MASKED_VALUE,
+ masked.get("jdbc-password"));
+ }
+ }
+
+ @Test
+ void testAssembleRejectsMaskedPlaceholderPlaintext() {
+ try (SecretManager sm = memorySecretManager()) {
+ Map<String, String> entityProps = new HashMap<>();
+ Map<String, SecretBinding> bindings =
+ Map.of(
+ "jdbc-password",
+ new SecretBinding(
+ "memory",
org.apache.gravitino.connector.HiddenPropertyMaskUtils.MASKED_VALUE));
+ IllegalArgumentException ex =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ sm.assembleSecretMaterials(
+ Map.of(), entityProps, "catalog", 42L, bindings,
Map.of()));
+ Assertions.assertTrue(ex.getMessage().contains("masked placeholder"));
+ Assertions.assertTrue(ex.getMessage().contains("jdbc-password"));
+ }
+ }
+
@Test
void testRejectReferenceUrns() {
try (SecretManager sm = memorySecretManager()) {
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/mapper/JsonMappingExceptionMapper.java
b/server/src/main/java/org/apache/gravitino/server/web/mapper/JsonMappingExceptionMapper.java
index df972d0ffa..3bdc32f33e 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/mapper/JsonMappingExceptionMapper.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/mapper/JsonMappingExceptionMapper.java
@@ -34,6 +34,8 @@ public class JsonMappingExceptionMapper implements
ExceptionMapper<JsonMappingEx
@Override
public Response toResponse(JsonMappingException e) {
String errorMsg = "Malformed json request";
- return
Utils.illegalArguments(IllegalArgumentException.class.getSimpleName(),
errorMsg, e);
+ // Do not attach the Jackson throwable: its message / stack can quote
request body fragments
+ // (for example a bare-string secretBindings value that is the secret
plaintext).
+ return
Utils.illegalArguments(IllegalArgumentException.class.getSimpleName(),
errorMsg, null);
}
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java
index 80ee553cd5..c58559c303 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java
@@ -816,7 +816,8 @@ public class ExceptionHandlers {
@Override
public Response handle(OperationType op, String secret, String parent,
Exception e) {
String reason = e instanceof ServiceUnavailableException ?
e.getMessage() : getErrorMsg(e);
- String errorMsg = getSecretErrorMsg(parent, reason);
+ // metadataObjectName is passed as {@code secret}; parent is unused for
this handler.
+ String errorMsg = getSecretErrorMsg(secret, reason);
LOG.warn(errorMsg, e);
if (e instanceof IllegalArgumentException) {
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectSecretOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectSecretOperations.java
index c294123a8d..45689d3739 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectSecretOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectSecretOperations.java
@@ -29,7 +29,6 @@ import java.util.Set;
import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.GET;
-import javax.ws.rs.NotSupportedException;
import javax.ws.rs.Path;
import javax.ws.rs.PathParam;
import javax.ws.rs.Produces;
@@ -105,7 +104,7 @@ public class MetadataObjectSecretOperations {
MetadataObjects.parse(
fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
if (!supportsSecretOperations(object)) {
- throw new NotSupportedException(
+ throw new IllegalArgumentException(
"Doesn't support secret property operations for metadata
object type");
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/mapper/TestJsonMappingExceptionMapper.java
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestJsonMappingExceptionMapper.java
index 96ca84017b..45b35d03e1 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/mapper/TestJsonMappingExceptionMapper.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestJsonMappingExceptionMapper.java
@@ -34,12 +34,15 @@ public class TestJsonMappingExceptionMapper {
public void testJsonMappingExceptionMapper() {
JsonParser mockParser = Mockito.mock(JsonParser.class);
Response response =
-
jsonMappingExceptionMapper.toResponse(JsonMappingException.from(mockParser,
""));
+ jsonMappingExceptionMapper.toResponse(
+ JsonMappingException.from(mockParser, "from String value
('MY-SECRET-VALUE-123')"));
ErrorResponse errorResponse = ErrorResponse.illegalArguments("Malformed
json request");
ErrorResponse entity = (ErrorResponse) response.getEntity();
Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
Assertions.assertEquals(errorResponse.getCode(), entity.getCode());
Assertions.assertEquals(errorResponse.getMessage(), entity.getMessage());
Assertions.assertEquals(errorResponse.getType(), entity.getType());
+ Assertions.assertNull(entity.getStack());
+
Assertions.assertFalse(entity.getMessage().contains("MY-SECRET-VALUE-123"));
}
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestExceptionHandlers.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestExceptionHandlers.java
index 44fec2564f..4856395f27 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestExceptionHandlers.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestExceptionHandlers.java
@@ -175,6 +175,20 @@ public class TestExceptionHandlers {
});
}
+ @Test
+ void testUnsupportedSecretObjectTypeReturnsBadRequest() {
+ String reason = "Doesn't support secret property operations for metadata
object type";
+ try (Response response =
+ ExceptionHandlers.handleSecretException(
+ OperationType.GET, "probe_tag", new
IllegalArgumentException(reason))) {
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse error = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
error.getCode());
+ Assertions.assertTrue(error.getMessage().contains("probe_tag"));
+ Assertions.assertTrue(error.getMessage().contains(reason));
+ }
+ }
+
@Test
void testUnmodifiableOperationReturnsConflict() {
UnmodifiableStatisticException exception =
