Conversation
- button at the bottom + ui in new tab - changed favicon - changed font
deleted unused html comments
marcorosa
left a comment
marcorosa
left a comment
There was a problem hiding this comment.
just a few minor comments. Plus, please fix the pep8 style errors from the bot
|
@cabch I tested the branch and I have some comments:
|
aligned attacks results TODO: textattack
marcorosa
left a comment
marcorosa
left a comment
There was a problem hiding this comment.
Hello @cabch I have finished the review :D
You will find some comments in the files, plus a few paragraphs here below to clarify a few points that require more than 1 line of text to be expressed.
db model for Model
I think there is some confusion on the way you call the models once stored in the db. Indeed, "Attack Model" would be good if that wasn't already used in the individual attacks with a different meaning. If we want to align to the naming we currently have, then the model under attack is the "target model", whereas the "attack model" is an llm used in some attacks (like pyrit) to mutate the attack prompts (so, it's used as an "evil attacker" and not as a target).
save_to_db call
I think there is space for optimizing the way we call the save_to_db function. Indeed, the call is added to each attack implementation (i.e., each file in libs, plus the attack suite). This way, every time we add a new attack, we have to remember to also save the results to the db. I think we can keep it this way for now, but keeping it in mind for our possible future restructuring of the project. As an alternative, the call to save results to db could be done in the AttackSpecification, as part of the start method (but it would require to process the result there) or somewhere else.
gptfuzz
It's not very clear to me the way results are returned. On my side, I must run it once again to refresh my memory, but still what is currently returned as part of the AttackResult needs a revision.
AttackResult
It is not clear if we need to standardize the returns, because you haven't been consistent across all the attacks. If so, we can also consider switching AttackResult.details json/dictionary to a dataclass to be more rigorous.
Style comments
The style of imports needs to be reviewed. From PEP8 definitions:
Imports should be grouped in the following order:
1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.
In addition, each block should be sorted alphabetically.
| db.session.commit() | ||
| print("Results successfully saved to the database.") | ||
| return inserted_records | ||
| except Exception as e: |
There was a problem hiding this comment.
also for the exception blocks, 2 comments you have already seen :D
- use logger instead of print
- try to make the exception a little less generic, if possible
There was a problem hiding this comment.
done ✅ + same comments as for adding an utility function for exception handling ? avoiding to avoid cluttering the code every time error handling is needed... need to check again
There was a problem hiding this comment.
adding multiple exception in a row
change to attack success rate re added return result
|
I merge the current PR to start working on its dockerization, but there are some runtime errors when running the attacks (both via UI and docker) |
|
Working on the runtime exceptions in the
via agent ui
|
| vulnerability_type = attack_results.vulnerability_type.lower() | ||
| details = attack_results.details # JSON column | ||
| target_name = details.get('target_model') | ||
| target_name = details.get('target_model', '').lower() |
There was a problem hiding this comment.
does it works if we don't have the target model ? we deleted the tolower() and moved it below in case we cannot find the target name it doesn't crash
2 participants
added risk dashboard