diff --git a/CedarJava/CHANGELOG.md b/CedarJava/CHANGELOG.md index 283fbd26..81d7c07c 100644 --- a/CedarJava/CHANGELOG.md +++ b/CedarJava/CHANGELOG.md @@ -8,6 +8,7 @@ * Added Offset function support [#331](https://github.com/cedar-policy/cedar-java/pull/331) * Added PolicySet to JSON conversion API [#329](https://github.com/cedar-policy/cedar-java/pull/329) * Added Cedar Schema support for Entity Validation [#332](https://github.com/cedar-policy/cedar-java/pull/332) +* Added `PolicyParseException`, thrown by `PolicySet.parsePolicies` when policy text fails to parse. It is a subclass of `InternalException`, so existing `catch` blocks and `getMessage()` are unaffected, and adds `getDetailedErrors()` returning Cedar's structured diagnostics - source span, expected tokens, and help text - for each error. `getErrors()` now carries one entry per parse error rather than a single entry for the whole document [#367](https://github.com/cedar-policy/cedar-java/pull/367) ## 4.3.1 ### Added diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java b/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java index 19a0d5fb..0fd032df 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/DetailedError.java @@ -38,7 +38,7 @@ public class DetailedError { /** Severity */ @JsonProperty("severity") public final Optional severity; - /** Source labels (ranges) */ + /** Source labels (ranges); see {@link SourceLabel} for how to index them */ @JsonProperty("sourceLocations") public final ImmutableList sourceLocations; /** Related errors */ @@ -84,14 +84,44 @@ public enum Severity { Error, } + /** + * A region of the source text an error refers to, so callers can underline it. + * + *

The offsets are UTF-8 byte offsets, not {@code String} indices. Cedar produces + * them by counting bytes, while {@link String#substring(int, int)} counts UTF-16 chars. The + * two coincide only while the source is pure ASCII; a single non-ASCII character anywhere + * earlier in the document - an accented identifier, a non-Latin string literal, an emoji in + * a comment - shifts them apart, and slicing the {@code String} directly then either + * extracts the wrong region or throws {@link StringIndexOutOfBoundsException}. Slice the + * source's UTF-8 bytes instead: + * + * 

{@code
+     * byte[] bytes = source.getBytes(StandardCharsets.UTF_8);
+     * String offending = new String(bytes, label.start, label.end - label.start, StandardCharsets.UTF_8);
+     * }
+ * + *

Offsets are absolute within the whole text that was parsed, not relative to the policy + * containing the error, so they remain directly usable when several policies are parsed + * together. They carry no policy identity of their own: mapping an offset back to a + * particular policy statement is left to the caller. + * + *

A region may be empty ({@code start == end}), which happens when there is no extent to + * highlight - an unterminated string literal, for instance, reports the position the lexer + * stopped at. Renderers should treat zero width as a single caret rather than assuming at + * least one character to underline. + * + *

For a parse error the region covers the unexpected token, which is not always where a + * reader would place the mistake: a missing operand is reported at the token that followed + * it, possibly on a later line. + */ public static final class SourceLabel { /** Text of the label (if any) */ @JsonProperty("label") public final Optional label; - /** Start of the source location (in bytes) */ + /** Start of the source location, as a UTF-8 byte offset, inclusive. See {@link SourceLabel}. */ @JsonProperty("start") public final int start; - /** End of the source location (in bytes) */ + /** End of the source location, as a UTF-8 byte offset, exclusive. See {@link SourceLabel}. */ @JsonProperty("end") public final int end; diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java index 16c34fd2..bcee3aff 100644 --- a/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/InternalException.java @@ -41,6 +41,22 @@ public InternalException(String[] errors) { this.errors = new ArrayList<>(Arrays.asList(errors)); } + /** + * Internal exception whose message is not derived from its error list. + * + *

The other constructors build the message by joining {@code errors}, which ties the + * two together: splitting the list more finely necessarily changes the message. Subclasses + * that report each underlying error separately, but summarise them differently in the + * message, use this constructor to set the two independently. + * + * @param error the message, prefixed as in {@link #InternalException(String)} + * @param errors the individual error messages, for {@link #getErrors()} + */ + protected InternalException(String error, List errors) { + super("Internal error: " + error); + this.errors = new ArrayList<>(errors); + } + /** * Get errors. * diff --git a/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java new file mode 100644 index 00000000..1e1d85f6 --- /dev/null +++ b/CedarJava/src/main/java/com/cedarpolicy/model/exception/PolicyParseException.java @@ -0,0 +1,92 @@ +/* + * Copyright Cedar Contributors + * + * Licensed 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 + * + * https://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 com.cedarpolicy.model.exception; + +import com.cedarpolicy.CedarJson; +import com.cedarpolicy.model.DetailedError; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +/** + * Thrown when Cedar policy text fails to parse, carrying the structured diagnostics Cedar + * produced for each error. + * + *

Cedar reports a parse failure as one or more {@code miette} diagnostics: a message, the + * source span of the offending token, the tokens the parser expected there, and often help + * text. A single document may fail in several places, and every failure is reported. + * + *

Three accessors describe the same failure at increasing fidelity: + * + *

+ * + *

Extends {@link InternalException}, so callers that catch the general parse-or-evaluate + * failure are unaffected and need not know this type exists. + */ +public final class PolicyParseException extends InternalException { + + private static final TypeReference> ERROR_LIST = + new TypeReference>() { }; + + private final transient List detailedErrors; + + /** + * Construct from the JSON array of {@code DetailedError} the native layer serialises. + * + * @param message the value {@link #getMessage()} reports + * @param messages one message per parse error, for {@link #getErrors()} + * @param detailedErrorsJson JSON array of {@code DetailedError}; if it cannot be read, + * the exception still carries {@code message} and {@code messages}, and + * {@link #getDetailedErrors()} returns empty, so a serialisation change can never + * turn a parse error into a different failure + */ + public PolicyParseException(String message, String[] messages, String detailedErrorsJson) { + super(message, Arrays.asList(messages)); + this.detailedErrors = readDetailedErrors(detailedErrorsJson); + } + + private static List readDetailedErrors(String json) { + if (json == null || json.isEmpty()) { + return List.of(); + } + try { + List parsed = CedarJson.objectReader().forType(ERROR_LIST).readValue(json); + return parsed == null ? List.of() : List.copyOf(parsed); + } catch (JsonProcessingException | RuntimeException e) { + return List.of(); + } + } + + /** + * The structured diagnostics for each parse error, including source spans and help text. + * + * @return the diagnostics, or an empty list if none could be recovered + */ + public List getDetailedErrors() { + return Collections.unmodifiableList(detailedErrors); + } +} diff --git a/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java new file mode 100644 index 00000000..a52fcdc1 --- /dev/null +++ b/CedarJava/src/test/java/com/cedarpolicy/PolicyParseDiagnosticsTests.java @@ -0,0 +1,108 @@ +/* + * Copyright Cedar Contributors + * + * Licensed 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 + * + * https://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 com.cedarpolicy; + +import com.cedarpolicy.model.DetailedError; +import com.cedarpolicy.model.exception.InternalException; +import com.cedarpolicy.model.exception.PolicyParseException; +import com.cedarpolicy.model.policy.PolicySet; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +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; + +/** Parse failures carry Cedar's structured diagnostics, not just a flattened string. */ +public class PolicyParseDiagnosticsTests { + + @Test + public void parseFailureCarriesSourceSpanAndExpectedTokens() { + // An entity literal in the action slot: the scope needs `action == ...`. + String src = "forbid(principal, Foo::Action::\"Read\", resource);"; + PolicyParseException e = + assertThrows(PolicyParseException.class, () -> PolicySet.parsePolicies(src)); + + List details = e.getDetailedErrors(); + assertEquals(1, details.size()); + DetailedError error = details.get(0); + assertTrue(error.message.contains("unexpected token `::`"), error.message); + + assertEquals(1, error.sourceLocations.size()); + DetailedError.SourceLabel span = error.sourceLocations.get(0); + // The span must cover the offending `::`, so callers can underline it. + assertEquals(src.indexOf("::"), span.start); + assertEquals(src.indexOf("::") + 2, span.end); + assertTrue(span.label.orElse("").contains("expected"), span.label.toString()); + } + + @Test + public void everyParseErrorIsReportedNotJustTheFirst() { + // ParseErrors' Display prints only the first error, so the flattened path reported + // one string for the whole document. Both accessors now carry all of them. + String src = "forbid(principal, Foo::Action::\"A\", resource);\n" + + "permit(principal, action, resource) when { 1 + };"; + PolicyParseException e = + assertThrows(PolicyParseException.class, () -> PolicySet.parsePolicies(src)); + + assertEquals(2, e.getDetailedErrors().size()); + assertEquals(2, e.getErrors().size()); + // The message stays what Display gave it - the first error alone - so populating the + // list cannot widen the string that existing callers match on. + assertEquals("Internal error: Internal JNI Error: " + e.getErrors().get(0), e.getMessage()); + } + + @Test + public void messageIsUnchangedForBackCompat() { + // Callers branch on getMessage() and match it with anchored regexes, so the string + // stays exactly as the generic error path wrote it, "Internal JNI Error: " and all. + // The added detail is reached through the accessors instead. + PolicyParseException e = assertThrows(PolicyParseException.class, + () -> PolicySet.parsePolicies("forbid(principal, Foo::Action::\"Read\", resource);")); + + assertEquals("Internal error: Internal JNI Error: unexpected token `::`", e.getMessage()); + // getErrors() entries are the bare Cedar messages: the prefix described the binding + // rather than any one error, and is meaningless once the list is per-error. + assertEquals(List.of("unexpected token `::`"), e.getErrors()); + } + + @Test + public void helpTextSurvivesWhenCedarSuppliesIt() { + PolicyParseException e = assertThrows(PolicyParseException.class, + () -> PolicySet.parsePolicies("permit(principle, action, resource);")); + + DetailedError error = e.getDetailedErrors().get(0); + assertTrue(error.help.isPresent(), "expected help text for an invalid scope variable"); + assertTrue(error.help.get().contains("principal"), error.help.get()); + } + + @Test + public void remainsCatchableAsInternalException() { + // PolicyParseException extends InternalException so existing callers keep working. + InternalException e = assertThrows(InternalException.class, + () -> PolicySet.parsePolicies("permit(principal, action, resource)")); + assertFalse(e.getErrors().isEmpty()); + } + + @Test + public void validPolicySetStillParses() { + assertDoesNotThrow(() -> PolicySet.parsePolicies("permit(principal, action, resource);")); + } +} diff --git a/CedarJavaFFI/src/interface.rs b/CedarJavaFFI/src/interface.rs index b03841e6..b5c27932 100644 --- a/CedarJavaFFI/src/interface.rs +++ b/CedarJavaFFI/src/interface.rs @@ -22,13 +22,13 @@ use cedar_policy::ffi::{ }; use cedar_policy::{ ffi::{is_authorized_json_str, validate_json_str}, - Authorizer, Entities as CedarEntities, EntityUid, Policy, PolicySet, Request, Schema, SlotId, - Template, + Authorizer, Entities as CedarEntities, EntityUid, ParseErrors, Policy, PolicySet, Request, + Schema, SlotId, Template, }; use cedar_policy_formatter::{policies_str_to_pretty, Config}; use dashmap::DashMap; use jni::{ - objects::{JClass, JObject, JString, JValueGen, JValueOwned}, + objects::{JClass, JObject, JString, JThrowable, JValueGen, JValueOwned}, sys::{jstring, jvalue}, JNIEnv, }; @@ -534,6 +534,101 @@ struct JavaInterfaceCall { arguments: String, } +/// Throw a `PolicyParseException` carrying Cedar's structured diagnostics for each parse +/// error. +/// +/// `jni_failed` reduces any error to `format!("Internal JNI Error: {e}")`, which for a parse +/// failure discards everything `miette` recorded — the source span, the tokens the parser +/// expected, the help text — and, because `ParseErrors`' `Display` prints only its first +/// error, every subsequent error too. Parse failures are the errors a policy author is most +/// likely to hit and the ones where that detail matters most, so they are additionally +/// converted to `DetailedError` (the same representation the validation path already +/// returns) and handed to Java intact. +/// +/// The message is deliberately left as `jni_failed` would have written it, prefix and all: +/// callers are known to branch on `getMessage()` and to match it with anchored regexes, so +/// it is part of the API. `getErrors()` does gain one entry per parse error instead of a +/// single entry for the whole document, and those entries carry the bare Cedar message, +/// since the prefix describes the binding rather than any one error. +fn throw_parse_errors(env: &mut JNIEnv<'_>, errs: &ParseErrors) { + if env.exception_check().unwrap_or_default() { + return; // An exception is already in flight; let it propagate. + } + let details: Vec = errs.iter().map(DetailedError::from).collect(); + let messages: Vec = details.iter().map(|d| d.message.clone()).collect(); + let details_json = serde_json::to_string(&details).unwrap_or_default(); + let message = internal_error_message(errs); + + // Fall back to the generic path if any part of building the richer exception fails, so + // a parse error is never silently turned into a different kind of failure. + match build_parse_exception(env, &message, &messages, &details_json) { + Ok(exception) => { + if env.throw(exception).is_err() { + throw_internal(env, errs); + } + } + Err(_) => throw_internal(env, errs), + } +} + +/// Build `PolicyParseException(String message, String[] messages, String detailedErrorsJson)`. +fn build_parse_exception<'a>( + env: &mut JNIEnv<'a>, + message: &str, + messages: &[String], + details_json: &str, +) -> Result> { + let string_class = env.find_class("java/lang/String")?; + let messages_array = + env.new_object_array(messages.len() as i32, &string_class, JObject::null())?; + for (i, message) in messages.iter().enumerate() { + let jmessage = env.new_string(message)?; + env.set_object_array_element(&messages_array, i as i32, jmessage)?; + } + let jmessage = env.new_string(message)?; + let jdetails = env.new_string(details_json)?; + let exception = env.new_object( + "com/cedarpolicy/model/exception/PolicyParseException", + "(Ljava/lang/String;[Ljava/lang/String;Ljava/lang/String;)V", + &[ + JValueGen::Object(&jmessage), + JValueGen::Object(&messages_array), + JValueGen::Object(&jdetails), + ], + )?; + Ok(JThrowable::from(exception)) +} + +/// The message `jni_failed` writes for `errs`. The sole definition of that string, so the +/// richer exception and the fallback below can never drift apart. +fn internal_error_message(errs: &ParseErrors) -> String { + format!("Internal JNI Error: {errs}") +} + +/// Throw a plain `InternalException`, exactly as `jni_failed` would have. +fn throw_internal(env: &mut JNIEnv<'_>, errs: &ParseErrors) { + // Guard on a pending exception exactly as `jni_failed` does. This is not only for safety + // but is also the more useful behaviour: whatever the JVM already threw (an + // `OutOfMemoryError`, or a `NoClassDefFoundError` from a `.jar` that predates the `.so`) + // says far more about the failure than an `InternalException` naming a parse error would, + // and it propagates to the caller when this native method returns. + // + // The guard is also what makes the `unwrap` below sound. jni-rs refuses to make JNI calls + // while an exception is pending, so `throw_new` would fail its internal `find_class` and + // return `Error::JavaException`; unwrapping that panics, and unwinding out of the + // `extern "C"` boundary that `jni_fn` generates would abort the JVM. + if env.exception_check().unwrap_or_default() { + return; + } + // We have to unwrap here as we're doing exception handling + // If we don't have the heap space to create an exception, the only valid move is ending the process + env.throw_new( + "com/cedarpolicy/model/exception/InternalException", + internal_error_message(errs), + ) + .unwrap(); +} + fn jni_failed(env: &mut JNIEnv<'_>, e: &dyn Error) -> jvalue { // If we already generated an exception, then let that go up the stack // Otherwise, generate a cedar InternalException and return null @@ -655,7 +750,13 @@ fn policy_set_to_json_internal<'a>( #[jni_fn("com.cedarpolicy.model.policy.PolicySet")] pub fn parsePoliciesJni<'a>(mut env: JNIEnv<'a>, _: JClass, policies_jstr: JString<'a>) -> jvalue { match parse_policies_internal(&mut env, policies_jstr) { - Err(e) => jni_failed(&mut env, e.as_ref()), + Err(e) => match e.downcast_ref::() { + Some(parse_errors) => { + throw_parse_errors(&mut env, parse_errors); + JValueOwned::Object(JObject::null()).as_jni() + } + None => jni_failed(&mut env, e.as_ref()), + }, Ok(policies_set) => policies_set.as_jni(), } }