CoreyD97 / Burp-Montoya-Utilities

A collection of utilities for building extensions using Burp's Montoya API
GNU Affero General Public License v3.0
46 stars 8 forks source link

Some suggestions #2

Open irsdl opened 1 year ago

irsdl commented 1 year ago

1- Breaking the utility into multiple project: For example, one for reading/storing variables only which can make the library size smaller, and its name will explain what it does 2- Making the registerSetting optional so: 2.1- when a param is going to be read and it has not been registered before, it can accept some more params such as default value, type, visibility so it can basically register it if it really needs to and it has not been registered before. 2.2- The same when it is storing parameters. 2.3- Additionally, if it can have some defaults for example to use global or project and to also obtain the type automatically (primary types only if it is read, get the type from the object when it has the actual object), then if someone just want to read a param, it can return null or empty if it cannot find anything or throws exception if something goes horribly wrong while saving a setting. 3- Make passing the gson and log optional completely or ultimately make all the storage functionality to be accessed via static methods (not sure what it can break if this happens), so it does not need to be initialised.

irsdl commented 1 year ago

For the deserialisation to work properly, the metadata can be stored with the objects: https://stackoverflow.com/questions/19129711/how-to-expose-class-names-when-serializing-with-gson/19132243#19132243 As long as it only supports primary types and not custom types, it should be immune against most deserialization attacks.

CoreyD97 commented 11 months ago

Thanks for the suggestions.

  1. The library isn't huge, so splitting up the project would just add more complexity to the development. Especially if, for example, there's then changes to the preferences that would need corresponding changes to the UI code. Making sure you have compatible versions the libraries etc

  2. The things you have suggested would definitely be possible, though I don't see a huge benefit over the current implementation. Java is a statically typed language, so it makes sense (in my mind at least) to have to register your types beforehand. If we didn't register the type, when the extension is loaded and a setting is retrieved before it is set, the library wouldn't know what type to expect and how to deserialize it. Are there any other advantages to the structure you're proposing that would outweigh issues like this?

  3. Logging is optional, but GSON is needed for (de)serialization. Using the PreferenceFactory will automatically register a default Gson provider if a custom implementation is not necessary.

I imagine the last point was probably out of a lack of readme file, so hopefully its addition clears that one up :)

irsdl commented 11 months ago

Thanks Corey.

The 2nd one in particular makes things hairy for me as I feel like I have to repeat the code when I can perform the type conversion when the value is read or stored to pass the parameter type or by getting from reading the variable type (I know it is not a repeat as one is register and one is read/write). In Sharpener for example when I want to add a new capability (like an extension), registering variables can be difficult as I then have to put it somewhere else or to create a structure around it. I may need to create my own preferences there. I currently have this to do something similar in Sharpener: https://github.com/irsdl/BurpSuiteSharpenerEx/blob/main/src/main/java/ninja/burpsuite/libs/burp/generic/ExtendedPreferences.java

For the 3rd point, I think using import com.coreyd97.BurpExtenderUtilities.IGsonProvider; has solved the issue for me. Previously I had to add Gson in my gradle which was not ideal, but now I don't need it.

CoreyD97 commented 11 months ago

Hmm... I see what you're doing there. I can see some instances where this might be useful, but in all honesty it looks like this is more trouble than registering your parameters :)

For example, you either have to explicitly provide a fallback when retrieving your setting, or check if it was returned with an "uninitialized" ("", -1, false) value before using its result.

Similarly for setting values, you seem to need to provide he visibility each time?