Skip to content

Commit 30eb00f

Browse files
andrewmwells-amazonkhieta
andrewmwells-amazon
and
khieta
authored
Deny unknown fields in FFI. (#815) (#1041)
Signed-off-by: Andrew Wells <[email protected]> Co-authored-by: Kesha Hietala <[email protected]>
1 parent 8d0605a commit 30eb00f

File tree

6 files changed

+169
-14
lines changed

6 files changed

+169
-14
lines changed

‎cedar-policy/CHANGELOG.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ Cedar Language Version: 4.0
2222

2323
### Changed
2424

25-
- The API around `Request::new` has changed to remove the `Option`s
26-
around the entity type arguments.
25+
- The API around `Request::new` has changed to remove the `Option`s
26+
around the entity type arguments. See [RFC 55](https://github.com/cedar-policy/rfcs/blob/main/text/0055-remove-unspecified.md).
2727
- Significantly reworked all public-facing error types to address some issues
2828
and improve consistency. See issue #745.
2929
- Finalized the `ffi` module which was preview-released in 3.2.0.
@@ -35,6 +35,7 @@ Cedar Language Version: 4.0
3535
- Changed JSON schema parser so that `Set`, `Entity`, `Record`, and `Extension`
3636
can be common type names; updated the error message when common type names
3737
conflict with built-in primitive type names (#974, partially resolving #973)
38+
- Changed the FFI to error on typos or unexpected fields in the input JSON.
3839

3940
### Removed
4041

‎cedar-policy/src/ffi/is_authorized.rs‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ pub fn is_authorized_partial_json_str(json: &str) -> Result<String, serde_json::
156156
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
157157
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
158158
#[serde(rename_all = "camelCase")]
159+
#[serde(deny_unknown_fields)]
159160
pub struct Response {
160161
/// Authorization decision
161162
decision: Decision,
@@ -169,6 +170,7 @@ pub struct Response {
169170
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
170171
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
171172
#[serde(rename_all = "camelCase")]
173+
#[serde(deny_unknown_fields)]
172174
pub struct Diagnostics {
173175
/// Ids of the policies that contributed to the decision.
174176
/// If no policies applied to the request, this set will be empty.
@@ -239,6 +241,7 @@ impl Diagnostics {
239241
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
240242
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
241243
#[serde(rename_all = "camelCase")]
244+
#[serde(deny_unknown_fields)]
242245
pub struct AuthorizationError {
243246
/// Id of the policy where the error (or warning) occurred
244247
#[cfg_attr(feature = "wasm", tsify(type = "string"))]
@@ -291,6 +294,7 @@ impl From<cedar_policy_core::authorizer::AuthorizationError> for AuthorizationEr
291294
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
292295
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
293296
#[serde(rename_all = "camelCase")]
297+
#[serde(deny_unknown_fields)]
294298
pub struct ResidualResponse {
295299
decision: Option<Decision>,
296300
satisfied: HashSet<PolicyId>,
@@ -458,6 +462,7 @@ pub enum PartialAuthorizationAnswer {
458462
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
459463
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
460464
#[serde(rename_all = "camelCase")]
465+
#[serde(deny_unknown_fields)]
461466
pub struct AuthorizationCall {
462467
/// The principal taking action
463468
principal: EntityUid,
@@ -492,6 +497,7 @@ pub struct AuthorizationCall {
492497
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
493498
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
494499
#[serde(rename_all = "camelCase")]
500+
#[serde(deny_unknown_fields)]
495501
pub struct PartialAuthorizationCall {
496502
/// The principal taking action. If this field is empty, then the principal is unknown.
497503
principal: Option<EntityUid>,
@@ -594,7 +600,7 @@ impl AuthorizationCall {
594600
},
595601
_ => {
596602
// At least one of the `errs.push(e)` statements above must have been reached
597-
return build_error(errs, warnings);
603+
build_error(errs, warnings)
598604
}
599605
}
600606
}
@@ -680,7 +686,7 @@ impl PartialAuthorizationCall {
680686
},
681687
_ => {
682688
// At least one of the `errs.push(e)` statements above must have been reached
683-
return build_error(errs, warnings);
689+
build_error(errs, warnings)
684690
}
685691
}
686692
}
@@ -1642,8 +1648,7 @@ mod partial_test {
16421648
"ID1": "permit(principal == User::\"alice\", action, resource);"
16431649
}
16441650
},
1645-
"entities": [],
1646-
"partial_evaluation": true
1651+
"entities": []
16471652
});
16481653

16491654
assert_is_authorized_json_partial(call);
@@ -1666,8 +1671,7 @@ mod partial_test {
16661671
"ID1": "permit(principal == User::\"alice\", action, resource);"
16671672
}
16681673
},
1669-
"entities": [],
1670-
"partial_evaluation": true
1674+
"entities": []
16711675
});
16721676

16731677
assert_is_not_authorized_json_partial(call);
@@ -1690,8 +1694,7 @@ mod partial_test {
16901694
"ID1": "permit(principal == User::\"alice\", action, resource);"
16911695
}
16921696
},
1693-
"entities": [],
1694-
"partial_evaluation": true
1697+
"entities": []
16951698
});
16961699

16971700
assert_is_residual(call, &HashSet::from(["ID1"]));
@@ -1714,8 +1717,7 @@ mod partial_test {
17141717
"ID1": "permit(principal, action, resource) when { principal == User::\"alice\" };"
17151718
}
17161719
},
1717-
"entities": [],
1718-
"partial_evaluation": true
1720+
"entities": []
17191721
});
17201722

17211723
assert_is_residual(call, &HashSet::from(["ID1"]));
@@ -1739,8 +1741,7 @@ mod partial_test {
17391741
"ID2": "forbid(principal, action, resource) unless { resource == Photo::\"door\" };"
17401742
}
17411743
},
1742-
"entities": [],
1743-
"partial_evaluation": true
1744+
"entities": []
17441745
});
17451746

17461747
assert_is_residual(call, &HashSet::from(["ID1"]));

‎cedar-policy/src/ffi/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,4 @@ mod utils;
2222
pub use utils::*;
2323
mod validate;
2424
pub use validate::*;
25+
mod tests;

‎cedar-policy/src/ffi/tests.rs‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
#[cfg(test)]
2+
mod ffi_tests {
3+
#[cfg(feature = "partial-eval")]
4+
use crate::ffi::is_authorized_partial_json;
5+
use crate::ffi::{is_authorized_json, validate_json};
6+
use cool_asserts::assert_matches;
7+
8+
#[test]
9+
fn test_fail_unknown_field_policy_slice() {
10+
let json = serde_json::json!({
11+
"principal": {
12+
"type": "User",
13+
"id": "alice"
14+
},
15+
"action": {
16+
"type": "Photo",
17+
"id": "view"
18+
},
19+
"resource": {
20+
"type": "Photo",
21+
"id": "door"
22+
},
23+
"context": {},
24+
"slice": {
25+
"policies": {},
26+
"templatePolicies": {},
27+
"entities": []
28+
}
29+
});
30+
31+
assert_matches!(is_authorized_json(json), Err(e) => {
32+
assert_eq!(e.to_string(), "unknown field `slice`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
33+
});
34+
}
35+
36+
#[test]
37+
fn test_fail_unknown_field_enable_request_validation() {
38+
let json = serde_json::json!({
39+
"principal": {
40+
"type": "User",
41+
"id": "alice"
42+
},
43+
"action": {
44+
"type": "Photo",
45+
"id": "view"
46+
},
47+
"resource": {
48+
"type": "Photo",
49+
"id": "door"
50+
},
51+
"context": {},
52+
"policies": {},
53+
"entities": [],
54+
"enableRequestValidation": true,
55+
});
56+
57+
assert_matches!(is_authorized_json(json), Err(e) => {
58+
assert_eq!(e.to_string(), "unknown field `enableRequestValidation`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
59+
});
60+
}
61+
62+
#[test]
63+
fn test_fail_unknown_field_policies() {
64+
let json = serde_json::json!({
65+
"principal": {
66+
"type": "User",
67+
"id": "alice"
68+
},
69+
"action": {
70+
"type": "Photo",
71+
"id": "view"
72+
},
73+
"resource": {
74+
"type": "Photo",
75+
"id": "door"
76+
},
77+
"context": {},
78+
"policies": {
79+
"policies": {}
80+
},
81+
"entities": []
82+
});
83+
84+
assert_matches!(is_authorized_json(json), Err(e) => {
85+
assert_eq!(e.to_string(), "unknown field `policies`, expected one of `staticPolicies`, `templates`, `templateLinks`");
86+
});
87+
}
88+
89+
#[cfg(feature = "partial-eval")]
90+
#[test]
91+
fn test_fail_unknown_field_partial_evaluation() {
92+
let json = serde_json::json!({
93+
"principal": {
94+
"type": "User",
95+
"id": "alice"
96+
},
97+
"action": {
98+
"type": "Photo",
99+
"id": "view"
100+
},
101+
"context": {},
102+
"policies": {
103+
"staticPolicies": {
104+
"ID1": "permit(principal == User::\"alice\", action, resource);"
105+
}
106+
},
107+
"entities": [],
108+
"partial_evaluation": true
109+
});
110+
111+
assert_matches!(is_authorized_partial_json(json), Err(e) => {
112+
assert_eq!(e.to_string(), "unknown field `partial_evaluation`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
113+
});
114+
}
115+
116+
#[test]
117+
fn test_fail_unknown_field_validation() {
118+
let json = serde_json::json!({
119+
"schema": { "json": { "": {
120+
"entityTypes": {
121+
"User": {
122+
"memberOfTypes": [ ]
123+
},
124+
"Photo": {
125+
"memberOfTypes": [ ]
126+
}
127+
},
128+
"actions": {
129+
"viewPhoto": {
130+
"appliesTo": {
131+
"resourceTypes": [ "Photo" ],
132+
"principalTypes": [ "User" ]
133+
}
134+
}
135+
}
136+
}}},
137+
"Policies": "forbid(principal, action, resource);permit(principal == Photo::\"photo.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice\");"
138+
});
139+
140+
assert_matches!(validate_json(json), Err(e) => {
141+
assert_eq!(e.to_string(), "unknown field `Policies`, expected one of `validationSettings`, `schema`, `policies`");
142+
});
143+
}
144+
}

‎cedar-policy/src/ffi/utils.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ extern crate tsify;
3333
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
3434
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
3535
#[serde(rename_all = "camelCase")]
36+
#[serde(deny_unknown_fields)]
3637
pub struct DetailedError {
3738
/// Main error message, including both the `miette` "message" and the
3839
/// `miette` "causes" (uses `miette`'s default `Display` output)
@@ -84,6 +85,7 @@ impl From<miette::Severity> for Severity {
8485
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
8586
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
8687
#[serde(rename_all = "camelCase")]
88+
#[serde(deny_unknown_fields)]
8789
pub struct SourceLabel {
8890
/// Text of the label (if any)
8991
pub label: Option<String>,
@@ -97,6 +99,7 @@ pub struct SourceLabel {
9799
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
98100
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
99101
#[serde(rename_all = "camelCase")]
102+
#[serde(deny_unknown_fields)]
100103
pub struct SourceLocation {
101104
/// Start of the source location (in bytes)
102105
pub start: usize,
@@ -408,6 +411,7 @@ impl Default for StaticPolicySet {
408411
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
409412
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
410413
#[serde(rename_all = "camelCase")]
414+
#[serde(deny_unknown_fields)]
411415
pub struct TemplateLink {
412416
/// Id of the template to link against
413417
template_id: PolicyId,
@@ -441,6 +445,7 @@ impl TemplateLink {
441445
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
442446
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
443447
#[serde(rename_all = "camelCase")]
448+
#[serde(deny_unknown_fields)]
444449
pub struct PolicySet {
445450
/// static policies
446451
#[serde(default)]

‎cedar-policy/src/ffi/validate.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ pub fn validate_json_str(json: &str) -> Result<String, serde_json::Error> {
104104
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
105105
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
106106
#[serde(rename_all = "camelCase")]
107+
#[serde(deny_unknown_fields)]
107108
pub struct ValidationCall {
108109
/// Validation settings
109110
#[serde(default)]
@@ -154,6 +155,7 @@ impl ValidationCall {
154155
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
155156
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
156157
#[serde(rename_all = "camelCase")]
158+
#[serde(deny_unknown_fields)]
157159
pub struct ValidationSettings {
158160
/// Whether validation is enabled. If this flag is set to `false`, then
159161
/// only parsing is performed. The default value is `true`.
@@ -176,6 +178,7 @@ impl Default for ValidationSettings {
176178
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
177179
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
178180
#[serde(rename_all = "camelCase")]
181+
#[serde(deny_unknown_fields)]
179182
pub struct ValidationError {
180183
/// Id of the policy where the error (or warning) occurred
181184
#[cfg_attr(feature = "wasm", tsify(type = "string"))]

0 commit comments

Comments
 (0)