Skip to content

Harden input validation for empty/whitespace string fields across request builders and type constructors #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Harden input validation for empty/whitespace string fields across request builders and type constructors #14

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6816f1c
Initial plan
Copilot Apr 11, 2026
2d522ac
Harden input validation for string fields (empty/whitespace/invalid)
Copilot Apr 11, 2026
f3d05b1
Address PR feedback: fix TryFrom bypasses, extract helper, privatize …
Copilot Apr 11, 2026
db3ef42
Merge origin/main into PR branch — resolve permissions.rs conflicts
Copilot Apr 11, 2026
745601b
Merge origin/main — resolve validation vs pagination conflicts, fix n…
Copilot Apr 11, 2026
dfeb732
Merge origin/main (ours strategy) — establish cbfd384 as ancestor to …
Copilot Apr 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions src/client/permissions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ impl<'a> std::future::IntoFuture for CheckPermissionRequest<'a> {

fn into_future(self) -> Self::IntoFuture {
Box::pin(async move {
if self.permission.trim().is_empty() {
return Err(Error::InvalidArgument(
"permission must not be empty".into(),
));
}

Comment on lines 71 to +74
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same trim().is_empty() validation (with identical error strings) is now duplicated across multiple request builders in this module. Consider factoring this into a small helper (e.g., validate_non_empty(field, value)) to avoid message drift and to keep future validations consistent.

Copilot uses AI. Check for mistakes.
let proto_req = proto::CheckPermissionRequest {
consistency: self.consistency,
resource: Some(self.resource),
Expand Down Expand Up @@ -248,6 +254,17 @@ impl<'a> LookupResourcesRequest<'a> {
pub async fn send(
self,
) -> Result<impl Stream<Item = Result<LookupResourceResult, Error>>, Error> {
if self.resource_type.trim().is_empty() {
return Err(Error::InvalidArgument(
"resource_type must not be empty".into(),
));
}
if self.permission.trim().is_empty() {
return Err(Error::InvalidArgument(
"permission must not be empty".into(),
));
}

let proto_req = proto::LookupResourcesRequest {
consistency: self.consistency,
resource_object_type: self.resource_type,
Expand Down Expand Up @@ -315,6 +332,17 @@ impl<'a> LookupSubjectsRequest<'a> {
pub async fn send(
self,
) -> Result<impl Stream<Item = Result<LookupSubjectResult, Error>>, Error> {
if self.permission.trim().is_empty() {
return Err(Error::InvalidArgument(
"permission must not be empty".into(),
));
}
if self.subject_type.trim().is_empty() {
return Err(Error::InvalidArgument(
"subject_type must not be empty".into(),
));
}

let proto_req = proto::LookupSubjectsRequest {
consistency: self.consistency,
resource: Some(self.resource),
Expand Down Expand Up @@ -433,6 +461,12 @@ impl<'a> std::future::IntoFuture for ExpandPermissionTreeRequest<'a> {

fn into_future(self) -> Self::IntoFuture {
Box::pin(async move {
if self.permission.trim().is_empty() {
return Err(Error::InvalidArgument(
"permission must not be empty".into(),
));
}

let proto_req = proto::ExpandPermissionTreeRequest {
consistency: self.consistency,
resource: Some(self.resource),
Expand Down Expand Up @@ -583,3 +617,105 @@ impl Client {
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::{Client, ObjectReference, SubjectReference};

/// Creates a throwaway `Client` pointed at a dummy address using a lazy
/// channel that does not connect until the first RPC attempt.
fn dummy_client() -> Client {
let channel = tonic::transport::Endpoint::from_static("http://127.0.0.1:1")
.connect_lazy();
Client::from_channel(channel, "test-token").expect("client construction failed")
}

#[tokio::test]
async fn check_permission_empty_permission_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let subject = SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap();
let err = c.check_permission(&resource, "", &subject).await.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[tokio::test]
async fn check_permission_whitespace_permission_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let subject = SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap();
let err = c.check_permission(&resource, " ", &subject).await.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[tokio::test]
async fn lookup_resources_empty_permission_rejected() {
let c = dummy_client();
let subject = SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap();
let result = c.lookup_resources("document", "", &subject).send().await;
assert!(matches!(result, Err(Error::InvalidArgument(_))));
}

#[tokio::test]
async fn lookup_resources_empty_resource_type_rejected() {
let c = dummy_client();
let subject = SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap();
let result = c.lookup_resources("", "view", &subject).send().await;
assert!(matches!(result, Err(Error::InvalidArgument(_))));
}

#[tokio::test]
async fn lookup_subjects_empty_permission_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let result = c.lookup_subjects(&resource, "", "user").send().await;
assert!(matches!(result, Err(Error::InvalidArgument(_))));
}

#[tokio::test]
async fn lookup_subjects_empty_subject_type_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let result = c.lookup_subjects(&resource, "view", "").send().await;
assert!(matches!(result, Err(Error::InvalidArgument(_))));
}

#[tokio::test]
async fn expand_permission_tree_empty_permission_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let err = c
.expand_permission_tree(&resource, "")
.await
.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[tokio::test]
async fn expand_permission_tree_whitespace_permission_rejected() {
let c = dummy_client();
let resource = ObjectReference::new("document", "doc1").unwrap();
let err = c
.expand_permission_tree(&resource, " ")
.await
.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}
}
73 changes: 65 additions & 8 deletions src/types/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,21 @@ pub struct RelationshipFilter {

impl RelationshipFilter {
/// Creates a new filter for the given resource type.
pub fn new(resource_type: impl Into<String>) -> Self {
Self {
resource_type: resource_type.into(),
///
/// Returns `Err` if `resource_type` is empty or whitespace-only.
pub fn new(resource_type: impl Into<String>) -> Result<Self, Error> {
let resource_type = resource_type.into();
Comment on lines 20 to +24
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RelationshipFilter::new now validates resource_type, but the struct still exposes pub resource_type, so callers can bypass validation via a struct literal. If the intent is to guarantee non-empty types across the API, consider making the field private (as with ObjectReference) and providing accessors/builders instead.

Copilot uses AI. Check for mistakes.
if resource_type.trim().is_empty() {
return Err(Error::InvalidArgument(
"resource_type must not be empty".into(),
));
}
Ok(Self {
resource_type,
optional_resource_id: None,
optional_relation: None,
optional_subject_filter: None,
}
})
}

/// Adds a resource ID filter.
Expand Down Expand Up @@ -71,12 +79,20 @@ pub struct SubjectFilter {

impl SubjectFilter {
/// Creates a new subject filter for the given type.
pub fn new(subject_type: impl Into<String>) -> Self {
Self {
subject_type: subject_type.into(),
///
/// Returns `Err` if `subject_type` is empty or whitespace-only.
pub fn new(subject_type: impl Into<String>) -> Result<Self, Error> {
Comment on lines 100 to +104
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SubjectFilter::new now validates subject_type, but the struct still exposes pub subject_type, so invalid values can still be created via struct literals (bypassing the constructor). If you want the invariant to hold everywhere, make the field private and expose accessors/builders instead.

Copilot uses AI. Check for mistakes.
let subject_type = subject_type.into();
if subject_type.trim().is_empty() {
return Err(Error::InvalidArgument(
"subject_type must not be empty".into(),
));
}
Ok(Self {
subject_type,
optional_subject_id: None,
optional_relation: None,
}
})
}

/// Adds a subject ID filter.
Expand Down Expand Up @@ -133,3 +149,44 @@ impl ReadRelationshipResult {
})
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn relationship_filter_valid() {
let f = RelationshipFilter::new("document").unwrap();
assert_eq!(f.resource_type, "document");
}

#[test]
fn relationship_filter_empty_type_rejected() {
let err = RelationshipFilter::new("").unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn relationship_filter_whitespace_type_rejected() {
let err = RelationshipFilter::new(" ").unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn subject_filter_valid() {
let f = SubjectFilter::new("user").unwrap();
assert_eq!(f.subject_type, "user");
}

#[test]
fn subject_filter_empty_type_rejected() {
let err = SubjectFilter::new("").unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn subject_filter_whitespace_type_rejected() {
let err = SubjectFilter::new(" ").unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}
}
82 changes: 71 additions & 11 deletions src/types/relationship.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,19 @@ pub struct Caveat {

impl Caveat {
/// Creates a new caveat with the given name and context.
pub fn new(name: impl Into<String>, context: HashMap<String, ContextValue>) -> Self {
Self {
name: name.into(),
context,
///
/// Returns `Err` if `name` is empty or whitespace-only.
pub fn new(name: impl Into<String>, context: HashMap<String, ContextValue>) -> Result<Self, Error> {
let name = name.into();
if name.trim().is_empty() {
return Err(Error::InvalidArgument(
"caveat name must not be empty".into(),
));
}
Ok(Self {
name,
context,
})
}
Comment on lines 17 to 32
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caveat::new now enforces a non-empty name, but TryFrom<proto::Relationship> (below) still constructs Caveat { name: ... } directly, bypassing this validation. Consider switching that conversion to call Caveat::new(...) (and propagate the error) so invariants hold for values coming from protobufs as well.

Copilot uses AI. Check for mistakes.
}

Expand All @@ -39,17 +47,25 @@ pub struct Relationship {

impl Relationship {
/// Creates a new relationship without a caveat.
///
/// Returns `Err` if `relation` is empty or whitespace-only.
pub fn new(
resource: ObjectReference,
relation: impl Into<String>,
subject: SubjectReference,
) -> Self {
Self {
) -> Result<Self, Error> {
let relation = relation.into();
if relation.trim().is_empty() {
return Err(Error::InvalidArgument(
"relation must not be empty".into(),
));
}
Ok(Self {
resource,
relation: relation.into(),
relation,
subject,
optional_caveat: None,
}
})
}
Comment on lines 48 to 69
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relationship::new now rejects empty/whitespace relation, but TryFrom<crate::proto::Relationship> for Relationship currently builds Relationship { relation: proto.relation, ... } directly, which can reintroduce invalid state. To keep the type invariant consistent (similar to ObjectReference::try_from calling ObjectReference::new), update the conversion to use Relationship::new(...) (and also validate any embedded caveat via Caveat::new).

Copilot uses AI. Check for mistakes.

/// Attaches a caveat to this relationship.
Expand Down Expand Up @@ -259,7 +275,8 @@ mod tests {
None::<String>,
)
.unwrap(),
);
)
.unwrap();
let update = RelationshipUpdate::create(rel);
assert_eq!(update.operation, Operation::Create);
}
Expand All @@ -275,15 +292,58 @@ mod tests {
)
.unwrap(),
)
.with_caveat(Caveat::new("ip_check", HashMap::new()));
.unwrap()
.with_caveat(Caveat::new("ip_check", HashMap::new()).unwrap());
assert!(rel.optional_caveat.is_some());
assert_eq!(rel.optional_caveat.unwrap().name, "ip_check");
}

#[test]
fn relationship_empty_relation_rejected() {
let err = Relationship::new(
ObjectReference::new("doc", "1").unwrap(),
"",
SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap(),
)
.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn relationship_whitespace_relation_rejected() {
let err = Relationship::new(
ObjectReference::new("doc", "1").unwrap(),
" ",
SubjectReference::new(
ObjectReference::new("user", "alice").unwrap(),
None::<String>,
)
.unwrap(),
)
.unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn caveat_empty_name_rejected() {
let err = Caveat::new("", HashMap::new()).unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn caveat_whitespace_name_rejected() {
let err = Caveat::new(" ", HashMap::new()).unwrap_err();
assert!(matches!(err, Error::InvalidArgument(_)));
}

#[test]
fn precondition_must_exist() {
use crate::types::RelationshipFilter;
let p = Precondition::must_exist(RelationshipFilter::new("document"));
let p = Precondition::must_exist(RelationshipFilter::new("document").unwrap());
assert_eq!(p.operation, PreconditionOp::MustExist);
}
}
Loading