Add request validation #3
Conversation
github_notifications_handler.ml
Outdated
| | _ -> Error "" | ||
|
|
||
| let validate_header_env_var optional_env_var header = | ||
| Option.value_map ~default:(Error "") |
There was a problem hiding this comment.
If you don't want to have details about the error, use Error ()
There was a problem hiding this comment.
I do care later down on line 50, when i return an error message and am coerced on line 49 to return also of type (event_name, string) result
There was a problem hiding this comment.
empty string is not acceptable nonetheless, it should have relevant information
There was a problem hiding this comment.
Not returning empty strings anymore
github_notifications_handler.ml
Outdated
|
|
||
| let validate_notification_type notification_type = | ||
| match notification_type with | ||
| | "push" -> Ok Push_event | ||
| | "pull_request" -> Ok Pull_request_event | ||
| | "check_suite" -> Ok CI_run_event | ||
| | _ -> Error "Notification type not accepted" | ||
| | _ -> Error "" |
There was a problem hiding this comment.
You are discarding information that we can later use for debug
There was a problem hiding this comment.
Also, wouldn't you want to stop everything if something wrong with this header?
There was a problem hiding this comment.
I've made changes to accomodate @ygrek suggestions, so the code is simpler now and more straightforward. I'm not raising an exception, but no more operations will be done on the request after it's determined that the github event is unsupported.
github_notifications_handler.ml
Outdated
| let user_agent = extract_header validate_github_user_agent headers "User-agent" in | ||
| match notification_signature, user_agent, github_event with | ||
| | Ok _, Ok _, Ok _ -> github_event | ||
| | _, _, _ -> Error "Headers validation failed" |
There was a problem hiding this comment.
You could add info about why they failed here
github_notifications_handler.ml
Outdated
| | _ -> Error "" | ||
|
|
||
| let validate_header_env_var optional_env_var header = | ||
| Option.value_map ~default:(Error "") |
There was a problem hiding this comment.
empty string is not acceptable nonetheless, it should have relevant information
github_notifications_handler.ml
Outdated
| let github_event = extract_header validate_notification_type headers "X-GitHub-Event" in | ||
| github_event | ||
| let validate_github_request_signature = validate_header_env_var Configuration.Env.github_sha1_signature in |
There was a problem hiding this comment.
this looks overcomplicated (with passing callbacks around) for no reason - simple pattern match will be enough
request_handler.ml
Outdated
| with exn -> | ||
| ( match Headers.get headers "X-Github-Event" with | ||
| | Some event -> Error (Printf.sprintf "While parsing the %s payload: %s" event (Exn.to_string exn)) | ||
| | None -> Error (Printf.sprintf "While parsing the payload: %s" (Exn.to_string exn)) ) |
There was a problem hiding this comment.
remove duplication : sprintf "xxx %s payload" (match Headers.get with Some s -> s | None -> "")
Builds on top of, and is blocked by #2.
Implemented request validation.
Renamed the config module due to conflict with
Httpaf.Config