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.

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

Jump to bottom

Display position in TransformDuplicate errors #775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Display position in TransformDuplicate errors #775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 9 additions & 4 deletions source/interprocedural_analyses/taint/annotationParser.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,15 @@ let parse_sink ~allowed ?subkind name =
| _ -> Error (Format.sprintf "Unsupported taint sink `%s`" name)


let parse_transform ~allowed name =
match List.find allowed ~f:(TaintTransform.equal (TaintTransform.Named name)) with
| Some transform -> Ok transform
| None -> Error (Format.sprintf "Unsupported transform `%s`" name)
let parse_transform ~allowed reference_name =
match
List.find allowed ~f:(function
| TaintTransform.Named { name; _ } -> String.equal name reference_name
| _ -> false)
with
| Some (TaintTransform.Named { name; _ }) -> Ok (TaintTransform.Named { name; location = None })
| Some (TaintTransform.Sanitize _ as transform) -> Ok transform
| None -> Error (Format.sprintf "Unsupported transform `%s`" reference_name)


let parse_tito ~allowed_transforms ?subkind name =
Expand Down
40 changes: 29 additions & 11 deletions source/interprocedural_analyses/taint/taintConfiguration.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,9 +507,8 @@ module Heap = struct
"XSS";
]
in
let transforms =
List.map ~f:(fun name -> TaintTransform.Named name) ["DemoTransform"; "TestTransform"]
in
let named_transform name = TaintTransform.Named { name; location = None } in
let transforms = List.map ~f:named_transform ["DemoTransform"; "TestTransform"] in
let rules =
[
{
Expand Down Expand Up @@ -591,7 +590,7 @@ module Heap = struct
{
sources = [Sources.NamedSource "Test"];
sinks = [Sinks.NamedSink "Test"];
transforms = [TaintTransform.Named "TestTransform"];
transforms = [named_transform "TestTransform"];
code = 5011;
name = "Flow with one transform.";
message_format =
Expand All @@ -601,7 +600,7 @@ module Heap = struct
{
sources = [Sources.NamedSource "Test"];
sinks = [Sinks.NamedSink "Test"];
transforms = [TaintTransform.Named "TestTransform"; TaintTransform.Named "DemoTransform"];
transforms = [named_transform "TestTransform"; named_transform "DemoTransform"];
code = 5011;
name = "Flow with two transforms.";
message_format =
Expand Down Expand Up @@ -741,7 +740,10 @@ module Error = struct
name: string;
previous_location: JsonAst.LocationWithPath.t option;
}
| TransformDuplicate of string
| TransformDuplicate of {
name: string;
previous_location: JsonAst.LocationWithPath.t option;
}
| FeatureDuplicate of string
| InvalidRegex of {
regex: string;
Expand Down Expand Up @@ -839,7 +841,17 @@ module Error = struct
previous_location.path
JsonAst.Location.pp_start
previous_location.location
| TransformDuplicate name -> Format.fprintf formatter "Duplicate entry for transform: `%s`" name
| TransformDuplicate { name; previous_location = None } ->
Format.fprintf formatter "Duplicate entry for transform: `%s`" name
| TransformDuplicate { name; previous_location = Some previous_location } ->
Format.fprintf
formatter
"Duplicate entry for transform: `%s`, previous entry was at `%a:%a`"
name
PyrePath.pp
previous_location.path
JsonAst.Location.pp_start
previous_location.location
| FeatureDuplicate name -> Format.fprintf formatter "Duplicate entry for feature: `%s`" name
| InvalidRegex { regex; reason } ->
Format.fprintf formatter "Invalid regex `%s`: `%s`" regex reason
Expand Down Expand Up @@ -1033,7 +1045,11 @@ let from_json_list source_json_list =
~current_keys:(JsonAst.Json.Util.keys json)
~valid_keys:["name"; "comment"]
>>= fun () ->
json_string_member ~path "name" json >>= fun name -> Result.Ok (TaintTransform.Named name)
json_string_member_with_location ~path "name" json
>>= fun (name, location) ->
Result.Ok
(TaintTransform.Named
{ name; location = Some (JsonAst.LocationWithPath.create ~path ~location) })
in
let parse_transforms (path, json) =
array_member ~path "transforms" json
Expand Down Expand Up @@ -1645,9 +1661,11 @@ let validate ({ Heap.sources; sinks; transforms; features; rules; _ } as configu
~get_error:(fun { AnnotationParser.name; _ } previous_location ->
Error.SinkDuplicate { name; previous_location })
sinks;
ensure_list_unique
~get_name:TaintTransform.show
~get_error:(fun name -> Error.TransformDuplicate name)
ensure_list_unique_with_location
~get_key:TaintTransform.show
~get_location:TaintTransform.get_location
~get_error:(fun transform previous_location ->
Error.TransformDuplicate { name = TaintTransform.show transform; previous_location })
transforms;
ensure_list_unique
~get_name:Fn.id
Expand Down
5 changes: 4 additions & 1 deletion source/interprocedural_analyses/taint/taintConfiguration.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ module Error : sig
name: string;
previous_location: JsonParsing.JsonAst.LocationWithPath.t option;
}
| TransformDuplicate of string
| TransformDuplicate of {
name: string;
previous_location: JsonParsing.JsonAst.LocationWithPath.t option;
}
| FeatureDuplicate of string
| InvalidRegex of {
regex: string;
Expand Down
15 changes: 13 additions & 2 deletions source/interprocedural_analyses/taint/taintTransform.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@

open Core

type named_transform = {
name: string;
location: (JsonParsing.JsonAst.LocationWithPath.t option[@hash.ignore] [@sexp.opaque]);
}
[@@deriving compare, eq, hash, sexp]

type t =
| Named of string
| Named of named_transform
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't change this, since this is the actual transform used during the analysis. We don't need to keep the location around during the analysis.
We do something similar for sources and sinks: we use AnnotationParser.source_or_sink during model parsing but we actually use Sources.t or Sinks.t in the analysis.
TL, DR: I would define a new type in annotationParser.ml to represent those. or just use AnnotationParser.source_or_sink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes! Will make the change :)

(* Invariant: set is not empty. *)
| Sanitize of SanitizeTransformSet.t
[@@deriving compare, eq, hash, sexp]

let pp formatter = function
| Named transform -> Format.fprintf formatter "%s" transform
| Named { name; _ } -> Format.fprintf formatter "%s" name
| Sanitize transforms -> SanitizeTransformSet.pp formatter transforms


Expand All @@ -39,3 +45,8 @@ let is_sanitize_transforms = function
let get_sanitize_transforms = function
| Named _ -> None
| Sanitize sanitize -> Some sanitize


let get_location = function
| Named { location; _ } -> location
| Sanitize _ -> None
10 changes: 9 additions & 1 deletion source/interprocedural_analyses/taint/taintTransform.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,14 @@
* LICENSE file in the root directory of this source tree.
*)

type named_transform = {
name: string;
location: (JsonParsing.JsonAst.LocationWithPath.t option[@hash.ignore] [@sexp.opaque]);
}
[@@deriving compare, eq, hash, sexp]

type t =
| Named of string
| Named of named_transform
(* Invariant: set is not empty. *)
| Sanitize of SanitizeTransformSet.t
[@@deriving compare, eq, hash, sexp]
Expand All @@ -20,3 +26,5 @@ val is_named_transform : t -> bool
val is_sanitize_transforms : t -> bool

val get_sanitize_transforms : t -> SanitizeTransformSet.t option

val get_location : t -> JsonParsing.JsonAst.LocationWithPath.t option
9 changes: 5 additions & 4 deletions source/interprocedural_analyses/taint/test/annotationParserTest.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,15 @@ let test_parse_tito _ =
in
assert_equal ~cmp:String.equal expected error
in
let named_transform name = TaintTransform.Named { name; location = None } in
assert_parsed ~expected:Sinks.LocalReturn "LocalReturn";
assert_parsed ~expected:(Sinks.ParameterUpdate 2) "ParameterUpdate2";
assert_parsed
~allowed_transforms:[TaintTransform.Named "T"]
~allowed_transforms:[named_transform "T"]
~expected:
(Sinks.Transform
{
local = TaintTransforms.of_named_transforms [TaintTransform.Named "T"];
local = TaintTransforms.of_named_transforms [named_transform "T"];
global = TaintTransforms.empty;
base = Sinks.LocalReturn;
})
Expand All @@ -105,12 +106,12 @@ let test_parse_tito _ =
~subkind:"Subkind"
"foo";
assert_parse_error
~allowed_transforms:[TaintTransform.Named "T"]
~allowed_transforms:[named_transform "T"]
~expected:"Unsupported transform `U`"
~subkind:"U"
"Transform";
assert_parse_error
~allowed_transforms:[TaintTransform.Named "T"]
~allowed_transforms:[named_transform "T"]
~expected:"Tito transform requires name of the transform as parameter"
"Transform"

Expand Down
Loading