Skip to content

Commit 3f8386e

Browse files
author
Mish Jude
committed
Address type mismatch false positive by implementing context validation
cr: https://code.amazon.com/reviews/CR-200369282
1 parent 1ea71be commit 3f8386e

File tree

4 files changed

+253
-2
lines changed

4 files changed

+253
-2
lines changed

‎cedar-policy-core/src/ast/request.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,14 @@ pub trait RequestSchema {
612612
request: &Request,
613613
extensions: &Extensions<'_>,
614614
) -> Result<(), Self::Error>;
615+
616+
/// Validate the given `context`, returning `Err` if it fails validation
617+
fn validate_context<'a>(
618+
&self,
619+
context: &Context,
620+
action: &EntityUID,
621+
extensions: &Extensions<'a>,
622+
) -> std::result::Result<(), Self::Error>;
615623
}
616624

617625
/// A `RequestSchema` that does no validation and always reports a passing result
@@ -626,6 +634,17 @@ impl RequestSchema for RequestSchemaAllPass {
626634
) -> Result<(), Self::Error> {
627635
Ok(())
628636
}
637+
638+
fn validate_context<'a>(
639+
&self,
640+
_context: &Context,
641+
_action: &EntityUID,
642+
_extensions: &Extensions<'a>,
643+
) -> std::result::Result<(), Self::Error> {
644+
Ok(())
645+
}
646+
647+
629648
}
630649

631650
/// Wrapper around `std::convert::Infallible` which also implements

‎cedar-policy-validator/src/coreschema.rs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,44 @@ impl ast::RequestSchema for ValidatorSchema {
260260
}
261261
Ok(())
262262
}
263+
264+
/// Validate a context against a schema for a specific action
265+
fn validate_context<'a>(
266+
&self,
267+
context: &ast::Context,
268+
action: &ast::EntityUID,
269+
extensions: &Extensions<'a>,
270+
) -> std::result::Result<(), RequestValidationError> {
271+
// Following the same logic in validate_request
272+
// Get the action ID
273+
let action_arc = Arc::new(action.clone());
274+
let validator_action_id = self.get_action_id(&action_arc).ok_or_else(|| {
275+
request_validation_errors::UndeclaredActionError {
276+
action: action_arc.clone(),
277+
}
278+
})?;
279+
280+
// Validate entity UIDs in the context
281+
validate_euids_in_partial_value(
282+
&CoreSchema::new(&self),
283+
&context.clone().into(),
284+
)
285+
.map_err(RequestValidationError::InvalidEnumEntity)?;
286+
287+
// Typecheck the context against the expected context type
288+
let expected_context_ty = validator_action_id.context_type();
289+
if !expected_context_ty
290+
.typecheck_partial_value(&context.clone().into(), extensions)
291+
.map_err(RequestValidationError::TypeOfContext)?
292+
{
293+
return Err(request_validation_errors::InvalidContextError {
294+
context: context.clone(),
295+
action: action_arc,
296+
}
297+
.into());
298+
}
299+
Ok(())
300+
}
263301
}
264302

265303
impl ValidatorActionId {
@@ -305,6 +343,16 @@ impl ast::RequestSchema for CoreSchema<'_> {
305343
) -> Result<(), Self::Error> {
306344
self.schema.validate_request(request, extensions)
307345
}
346+
347+
/// Validate the given `context`, returning `Err` if it fails validation
348+
fn validate_context<'a>(
349+
&self,
350+
context: &cedar_policy_core::ast::Context,
351+
action: &EntityUID,
352+
extensions: &Extensions<'a>,
353+
) -> std::result::Result<(), Self::Error> {
354+
self.schema.validate_context(context, action, extensions)
355+
}
308356
}
309357

310358
/// Error when the request does not conform to the schema.

‎cedar-policy/src/api.rs‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ pub use ast::Effect;
4444
pub use authorizer::Decision;
4545
#[cfg(feature = "partial-eval")]
4646
use cedar_policy_core::ast::BorrowedRestrictedExpr;
47-
use cedar_policy_core::ast::{self, RestrictedExpr};
47+
use cedar_policy_core::ast::{self, RequestSchema, RestrictedExpr};
4848
use cedar_policy_core::authorizer;
4949
use cedar_policy_core::entities::{ContextSchema, Dereference};
5050
use cedar_policy_core::est::{self, TemplateLink};
@@ -4561,6 +4561,28 @@ impl Context {
45614561
) -> Result<Self, ContextCreationError> {
45624562
Self::from_pairs(self.into_iter().chain(other_context))
45634563
}
4564+
4565+
/// Validates this context against the provided schema
4566+
/// Returns Ok(()) if the context is valid according to the schema, or an error otherwise
4567+
pub fn validate_context(&self, schema: Option<&crate::Schema>, action: Option<&EntityUid>) -> std::result::Result<(), String> {
4568+
match (schema, action) {
4569+
(Some(schema), Some(action)) => {
4570+
// Get the context schema for this action
4571+
let _context_schema = Self::get_context_schema(schema, action)
4572+
.map_err(|e| format!("Context schema error: {}", e))?;
4573+
4574+
// Call the validate_context function from coreschema.rs
4575+
RequestSchema::validate_context(
4576+
&schema.0,
4577+
&self.0,
4578+
action.as_ref(),
4579+
&Extensions::all_available()
4580+
)
4581+
.map_err(|e| format!("Context validation failed: {}", e))
4582+
},
4583+
_ => Ok(()) // If no schema or action provided, we can't validate, so return Ok
4584+
}
4585+
}
45644586
}
45654587

45664588
/// Utilities for implementing `IntoIterator` for `Context`

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

Lines changed: 163 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,20 @@ pub fn check_parse_context(call: ContextParsingCall) -> CheckParseAnswer {
153153
};
154154
}
155155
};
156-
call.context.parse(schema.as_ref(), action.as_ref()).into()
156+
157+
let parse_result = call.context.parse(schema.as_ref(), action.as_ref());
158+
159+
// Check if the parsed context is valid
160+
if let Ok(context) = &parse_result {
161+
if let Err(err) = context.validate_context(schema.as_ref(), action.as_ref()) {
162+
return CheckParseAnswer::Failure {
163+
errors: vec![miette::Report::msg(err).into()],
164+
};
165+
}
166+
}
167+
168+
// Return the parse result if all other checks pass
169+
parse_result.into()
157170
}
158171

159172
/// Check whether a context successfully parses. Input is a JSON encoding of
@@ -534,4 +547,153 @@ mod test {
534547
let errs = assert_check_parse_is_err(&answer);
535548
assert_exactly_one_error(errs, "while parsing context, expected the record to have an attribute `referrer`, but it does not", None);
536549
}
550+
551+
#[test]
552+
fn check_parse_context_fails_for_invalid_context_type(){
553+
let call = json!({
554+
"context": {
555+
"authenticated": "foo"
556+
},
557+
"action": {
558+
"type": "PhotoApp::Action",
559+
"id": "viewPhoto"
560+
},
561+
"schema": {
562+
"PhotoApp": {
563+
"commonTypes": {
564+
"PersonType": {
565+
"type": "Record",
566+
"attributes": {
567+
"age": {
568+
"type": "Long"
569+
},
570+
"name": {
571+
"type": "String"
572+
}
573+
}
574+
},
575+
"ContextType": {
576+
"type": "Record",
577+
"attributes": {
578+
"ip": {
579+
"type": "Extension",
580+
"name": "ipaddr",
581+
"required": false
582+
},
583+
"authenticated": {
584+
"type": "Boolean",
585+
"required": true
586+
}
587+
}
588+
}
589+
},
590+
"entityTypes": {
591+
"User": {
592+
"shape": {
593+
"type": "Record",
594+
"attributes": {
595+
"userId": {
596+
"type": "String"
597+
},
598+
"personInformation": {
599+
"type": "PersonType"
600+
}
601+
}
602+
},
603+
"memberOfTypes": [
604+
"UserGroup"
605+
]
606+
},
607+
"UserGroup": {
608+
"shape": {
609+
"type": "Record",
610+
"attributes": {}
611+
}
612+
},
613+
"Photo": {
614+
"shape": {
615+
"type": "Record",
616+
"attributes": {
617+
"account": {
618+
"type": "Entity",
619+
"name": "Account",
620+
"required": true
621+
},
622+
"private": {
623+
"type": "Boolean",
624+
"required": true
625+
}
626+
}
627+
},
628+
"memberOfTypes": [
629+
"Album",
630+
"Account"
631+
]
632+
},
633+
"Album": {
634+
"shape": {
635+
"type": "Record",
636+
"attributes": {}
637+
}
638+
},
639+
"Account": {
640+
"shape": {
641+
"type": "Record",
642+
"attributes": {}
643+
}
644+
}
645+
},
646+
"actions": {
647+
"viewPhoto": {
648+
"appliesTo": {
649+
"principalTypes": [
650+
"User",
651+
"UserGroup"
652+
],
653+
"resourceTypes": [
654+
"Photo"
655+
],
656+
"context": {
657+
"type": "ContextType"
658+
}
659+
}
660+
},
661+
"createPhoto": {
662+
"appliesTo": {
663+
"principalTypes": [
664+
"User",
665+
"UserGroup"
666+
],
667+
"resourceTypes": [
668+
"Photo"
669+
],
670+
"context": {
671+
"type": "ContextType"
672+
}
673+
}
674+
},
675+
"listPhotos": {
676+
"appliesTo": {
677+
"principalTypes": [
678+
"User",
679+
"UserGroup"
680+
],
681+
"resourceTypes": [
682+
"Photo"
683+
],
684+
"context": {
685+
"type": "ContextType"
686+
}
687+
}
688+
}
689+
}
690+
}
691+
692+
}
693+
});
694+
let answer = serde_json::from_value(check_parse_context_json(call).unwrap()).unwrap();
695+
let errs = assert_check_parse_is_err(&answer);
696+
assert_exactly_one_error(errs, "Context validation failed: context `{authenticated: \"foo\"}` is not valid for `PhotoApp::Action::\"viewPhoto\"`", None);
697+
}
698+
537699
}

0 commit comments

Comments
 (0)