Skip to content
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.

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

Add validation to create evidence form #1115

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

Conversation

jkennedyvz
Copy link
Contributor

@jkennedyvz jkennedyvz commented Jun 1, 2024

Related to #1088

This pull request introduces validation logic to the CreateEvidenceModal component to ensure that both the description and evidence content fields are not empty before submission. This change addresses the issue of being able to submit blank evidence, enhancing the form's data integrity.

  • Adds a validateForm function that checks for non-empty description and evidence content. It generates an array of error messages if validation fails.
  • Modifies the handleSubmit function within useForm hook to include a call to validateForm. If validation errors exist, the submission is halted, and errors are displayed to the user.
  • Maintains the existing functionality of evidence type selection and file upload, ensuring that the validation logic seamlessly integrates without disrupting the user experience.

For more details, open the Copilot Workspace session.

@jrozner
Copy link
Member

jrozner commented Jun 1, 2024

Not sure about the react specific stuff here but we probably want to implement this on the backend rather than the frontend so that it's not possible to create on the API routes either. For quick feedback it might make sense to keep some frontend validation as well.

Implements frontend validation for creating evidence in `frontend/src/pages/operation_show/evidence_modals/index.tsx` and backend validation for evidence content in `backend/services/evidence.go` and `backend/services/evidence_metadata.go`.

- Adds a `validateForm` function to check for empty description and content fields before evidence creation on the frontend.
- Integrates form validation checks into the evidence creation process, rejecting the operation with an error if validation fails.
- Implements backend validation in `CreateEvidence` function to ensure the description is not empty and content is provided for evidence types other than 'event'.
- Adds validation in `CreateEvidenceMetadata` and `UpsertEvidenceMetadata` functions in `evidence_metadata.go` to ensure `source` and `body` fields are not empty, enhancing data integrity for evidence metadata operations.


---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ashirt-ops/ashirt-server/pull/1115?shareId=fe247a5c-57d0-4ff1-99ab-f871be2086b7).
@jkennedyvz
Copy link
Contributor Author

Implements frontend validation for creating evidence in frontend/src/pages/operation_show/evidence_modals/index.tsx and backend validation for evidence content in backend/services/evidence.go and backend/services/evidence_metadata.go.

  • Adds a validateForm function to check for empty description and content fields before evidence creation on the frontend.
  • Integrates form validation checks into the evidence creation process, rejecting the operation with an error if validation fails.
  • Implements backend validation in CreateEvidence function to ensure the description is not empty and content is provided for evidence types other than 'event'.
  • Adds validation in CreateEvidenceMetadata and UpsertEvidenceMetadata functions in evidence_metadata.go to ensure source and body fields are not empty, enhancing data integrity for evidence metadata operations.

For more details, open the Copilot Workspace session.

@jkennedyvz
Copy link
Contributor Author

There's probably a better solution than what copilot provided this first shot. Going to try some other attempts at this issue.

Copy link
Collaborator

@JoelAtDeluxe JoelAtDeluxe left a comment

Choose a reason for hiding this comment

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

You definitely need to fix evidence.go to move that higher in the function. You probably also want to update evidence_metadata.go to add in the check in updateEvidenceMetadata.

My mind is drifting to another solution in the dissectors, but I need to think about that a bit more.

@@ -161,6 +161,14 @@ func CreateEvidence(ctx context.Context, db *database.Connection, contentStore c
logging.Log(ctx, "msg", "Unable to run workers", "error", err.Error())
}

// Validation logic for description and content fields
if i.Description == "" {
return nil, backend.WrapError("Description cannot be empty.", backend.BadInputErr(nil, "Description cannot be empty."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. You don't need to use WrapError here. You can just use the BadInputErr
  2. You're doing this too low in the function. Two things to think about: First, you should be failing as soon as you know the operation won't work. Second, this needs to be done before it's written into the database (see lines 134 to 152 above).

@@ -43,6 +43,10 @@ func CreateEvidenceMetadata(ctx context.Context, db *database.Connection, i Edit
return backend.WrapError("Unwilling to create evidence metadata", backend.UnauthorizedWriteErr(err))
}

if i.Source == "" || i.Body == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's up to you if this is the way you want to do that, but generally you'd want to do these checks individually, as it'll provide more of an indication to the caller how to fix the issue.

You also forgot to add a similar check in updateEvidence. I'd need to check, but I suspect you could still create empty evidence if you wanted by creating evidence with source and body, and then editing the evidence to remove the source and body.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might also look at strings.TrimSpace

Comment on lines +94 to +103
const validateForm = () => {
const errors = []
if (descriptionField.value.trim() === "") {
errors.push("Description cannot be empty.")
}
if (getSelectedOption().value !== 'event' && binaryBlobField.value === null && codeblockField.value.code.trim() === "") {
errors.push("Evidence content cannot be empty.")
}
return errors
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to run this, but docker seems to be misconfigured on this box. But, I think this will be okay. I don't think we have a lot of validation rules elsewhere to pull from, but I'll take a look to see if we solved this in a different way elsewhere.

@JoelAtDeluxe
Copy link
Collaborator

One other thing that could be done would be to introduce validation at the dissector level. I didn't finish this, but it could look something like this:

// inside Coercable, we'd introduce some validator:
type ValidateFunc func(fieldName str, value interface{}) error

// Then, we'd create custom validators -- like this one to ensure a (string) field isn't empty...
func strNotEmpty(fieldName string, v interface{}) error {
  s, _ := v.(string)
  if strings.TrimSpace(s) == "" {
    return fmt.Errorf("%v cannot be empty", fieldName)
  }
  return nil
}

// we'd then use it when validating routes
	route(r, "POST", "/operations/{operation_slug}/evidence", jsonHandler(func(r *http.Request) (interface{}, error) {
		dr := dissectFormRequest(r)
		i := services.CreateEvidenceInput{
			Description:   dr.FromBody("description").Required().Validate(strNotEmpty).AsString(),
			//                                                   ^^^^^^^^^^^^^^^^^^^^^ Tell the dissector to flag this as an error if it fails validation
       // rest of the function

There are a couple of downsides:

  1. This works only at the field level, not at the request level -- so your check of contentType != event && content != nil won't work here.
  2. While some validation occurs here (specifically vetting the incoming shape of the data), most of the validation occurs within the handler, as it understand what is required to complete the function. Adding validation here could be seen as a breaking of this pattern
  3. The validation is somewhat inefficient, because it has to operate on data as an empty interface, rather than the real type, so in nearly every situation, we'll have to coerce the value back into the proper type (again).

Separately, we could look into using this (popular) library: https://github.com/go-playground/validator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants