-
Notifications
You must be signed in to change notification settings - Fork 2
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:framework-zap #42
base: main
Are you sure you want to change the base?
Conversation
|
||
# Verifica se o diretório de origem existe | ||
if [ ! -d "$ORIGEM" ]; then | ||
exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please log an error message so the user knows what has gone wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4ae37b6
logs were included in possible errors in copy.sh
exit 1 | |
# Check if the source directory exists | |
if [ ! -d "$SOURCE" ]; then | |
echo "Error: Source directory '$SOURCE' does not exist." |
Authentication_ZAP_GT_CRIVO/copy.sh
Outdated
@@ -0,0 +1,35 @@ | |||
#!/bin/bash | |||
|
|||
# Verifica se foi passado somente 1 argumento |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please translate all variable names, documentation, function names into English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all scripts and documentation have been translated into English
@@ -0,0 +1,35 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#!/bin/bash | |
#!/bin/bash | |
set -eu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested change was implemented.
Authentication_ZAP_GT_CRIVO/copy.sh
Outdated
fi | ||
|
||
# Verifica se o diretório de destino existe se não existir, criar | ||
docker compose exec framework bash -c "mkdir -p $DESTINO && rm -rf $DESTINO/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating the mount point is better done inside the Dockerfile
, as it's something the container needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't implement the functionality satisfactorily.
Authentication_ZAP_GT_CRIVO/copy.sh
Outdated
# comando que apaga todos os arquivos de configuração se houver antes de copiar os novos. | ||
|
||
# Copia apenas o conteúdo .json do diretório de origem para o volume | ||
for file in "$ORIGEM"/*.json; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this step. The docker-compose.yml
file is currently defining a named Docker volume (shared_data
) and mounting it inside the container on /shared_data
. We should instead mount a local directory inside the container (possibly dropping the underscore for conciseness), e.g.:
framework:
volumes:
- ./framework:/app
- ./shared:/shared:rw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about doing it the way you suggested, as we discussed, but it didn't work as well as the current approach, where the user provides the folder with the information and the script copies it to the volume.
- zaproxy | ||
volumes: | ||
- ./framework:/app | ||
- shared_data:/shared_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Share local directory for input/output (replace copy.sh
script, see comments in there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to change it, but the new approach didn't work correctly, so I kept the original one.
@@ -0,0 +1,20 @@ | |||
generate_key: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like file.env
is never used. Where is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .env
file is created to temporarily store the API key for Zap. After the configuration, a command is included in the .sh
script to delete the file.
generate_key: | |
rm file.env |
password: str | ||
``` | ||
|
||
Apenas dois atributos são opcionais: a URL da página de login e o título do relatório. Se a URL de login não for fornecida, o framework utiliza o spider do ZAP para tentar identificar as URLs da aplicação a partir da URL base. No entanto, nem sempre é possível encontrar a página de login automaticamente, por isso é recomendável fornecer essa URL sempre que possível. Se o spider falhar em encontrar a URL de login, um erro será lançado. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O que é o "título do relatório"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "context"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation was revised, and all the mentioned concepts, as well as those that previously lacked explanations, were properly clarified.
``` | ||
|
||
Apenas dois atributos são opcionais: a URL da página de login e o título do relatório. Se a URL de login não for fornecida, o framework utiliza o spider do ZAP para tentar identificar as URLs da aplicação a partir da URL base. No entanto, nem sempre é possível encontrar a página de login automaticamente, por isso é recomendável fornecer essa URL sempre que possível. Se o spider falhar em encontrar a URL de login, um erro será lançado. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the contents of example.json
here and remove the file from disk. Explain each of the fields. And provide an actual valid configuration... maybe for the Juice Shop application running on a container or VM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included a real-world example in the documentation, something like this.
context: "context_bWAPP"
url: "192.168.15.3/bWAPP/"
url_login: "192.168.15.3/bWAPP/login"
exclude_urls: []
report_title: "Report_bWAPP"
login: "admin"
password: "admin"
|
||
## Web Server | ||
|
||
O servidor web foi projetado para criar uma interface entre o framework e o usuário. Ele roda na porta `8000` e expõe o volume no localhost, permitindo que o usuário visualize os arquivos gerados. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a local folder is shared between the host and containers, then there is no need for a server to host the files. (See comments in docker-compose.yml
and copy.sh
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested, the server was removed from the final version of the framework.
} | ||
``` | ||
|
||
O Pydantic fará o *parser* do arquivo e extrairá os atributos necessários para a criação do contexto. Um exemplo será disponibilizado na raiz do repositório, chamado `example.json`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, mover essa parte para o ponto acima onde descrevemos a classe do Pydantic. Acho que se mantivermos o example.json é melhor colocá-lo dentro do ./shared
de uma vez.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it in the root of the application because the example is only copied to the volume when the application starts. How would the user have access to it before that?
Para iniciar o framework, utilize o comando: | ||
|
||
```bash | ||
make run DIR="diretório dos arquivos" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quais arquivos?
I think we can make this process much simpler using the ./shared
trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see https://github.com/cunha/easymapit for an example of a container that uses a shared directory to receive input and write output. It doesn't user Docker Compose, but the same mechanism applies. With plain Docker, the -v
is the switch we use to configure the volume in the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make it as user-friendly as possible. The user can have a folder anywhere in their workspace with the necessary files for running the tests. Once again, the documentation explains the process.
O `docker-compose` irá subir três containers: o ZAP, o framework e o servidor web. Os containers rodam em *background*. Para visualizar os logs, use os comandos: | ||
|
||
```bash | ||
docker logs -f authentication_zap_gt_crivo-zaproxy-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use plain docker logs
instead of docker compose logs <service>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Zap container generates a lot of output on the screen, so I preferred to separate both containers to make debugging clearer.
@@ -0,0 +1,28 @@ | |||
# Use a imagem oficial do Python como base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could try to drop this container entirely by using a shared volume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand the purpose.
@@ -0,0 +1,29 @@ | |||
annotated-types==0.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to include everything from a pip freeze
, but it would be better, for documentation reasons, to list only the dependencies. If you want to lock the version of all dependencies, then we could use a project management tool (I hear rye
is the hot thing these days) to create a lock file for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed libraries from the language and tried to keep the ones that are used through imports.
FIREFOX = os.getenv("FIREFOX") | ||
ZAP_API_KEY = os.getenv("ZAP_API_KEY") | ||
ZAP_PROXY_ADDRESS = os.getenv("ZAP_PROXY_ADDRESS") | ||
ZAP_PROXY_PORT = 8080 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use getenv
for the port number too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can provide a default with os.getenv("ZAP_PROXY_PORT", 8080)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! The environment variable is treated as an integer, so I just performed a type conversion, something like:
ZAP_PROXY_PORT = int(os.getenv("ZAP_PROXY_PORT"))
for element in elements: | ||
element_info = {} | ||
try: | ||
attributes_to_gather = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esses atributos são do HTML, certo? A pessoa tem flexibilidade para definir estes atributos ou são fixos do padrão HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the attributes can be generated by the developer, then should we have regexes to find the attributes too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version of the framework uses only HTML tags.
depends_on: | ||
- zaproxy | ||
volumes: | ||
- ./framework:/app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The framework
directory is already copied inside the container at /app
by the Dockerfile
, is this doing something different or the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to separate the setup of the two containers, the framework and ZAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you please elaborate?
Authentication_ZAP_GT_CRIVO/framework/keywords/regex_words/invalid_urls.txt
Show resolved
Hide resolved
password, credential_password, 1 | ||
) | ||
else: | ||
if "%40" in text: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cryptic. Why do we only replace @
with %40
when login != password
? Please write some documentation explaining what is being accomplished here and why it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explain what happens in the documentation: in some applications, the request body formats in a way that the @
becomes %40
.
Authentication_ZAP_GT_CRIVO/framework/autentication/autentication.py
Outdated
Show resolved
Hide resolved
print("failed to log in !") | ||
driver.refresh() | ||
return | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid naked except
clauses. It doesn't seem like the text above raises any exception, and the code seems also unclear how the function "returns" information to the caller (it either prints "failed to log in" or "login done"). How do we know what has happened outside the function?). Maybe rewrite the code so the intent is clearer or write some documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit seems unrelated to the issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function handles cases where the element cannot be found. If one of the elements corresponding to the form is incompatible, it will refresh the page to apply other elements during authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments inline on suggestions to improve. The most significant one is the handling of the volume for exchanging inputs and outputs with the container, which will significantly simplify usage.
…ete server and refactor readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parei a revisão na metade. Todos os comentários foram fechados com ponteiros para commits muitas vezes não relacionados ao comentário que eu tinha deixado. Se for possível, passe nos comentários que eu tinha feito elaborando as respostas ou fazendo modificações para adequar o código.
depends_on: | ||
- zaproxy | ||
volumes: | ||
- ./framework:/app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you please elaborate?
print("failed to log in !") | ||
driver.refresh() | ||
return | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit seems unrelated to the issue here
credential_login="{%username%}", | ||
credential_password="{%password%}", | ||
): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For docscrings, we should follow https://peps.python.org/pep-0257 so autogenerated documentation matches existing formatting templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forgot to use black in the code, now its done.
Pull request created, all basic information about the program's flow can be found in the README.