Feat/save responses to output directory provided in new -srd flag and also print if in json if -json flag enabled fixes #128, #129, #130 #161
Conversation
… in console as json output (for -json)
|
Hi @kartikeysemwal ! Thanks for your work and contribution. Really appreciated. Two actions are failing (go build and linter). Please fix them before I start a review : ) If you need any help/assistance I'm here ! |
|
@edoardottt updated according to the workflow errors, could you please rerun and check |
| } | ||
|
|
||
| config.OutputDir = output.CariddiOutputFolder | ||
| if flags.StoredRespDir != "" { |
There was a problem hiding this comment.
I think we should perform some tests. What if config.StoreResp is not specified?
There was a problem hiding this comment.
With the two tags -sr and -srd, there are four possible scenarios:
-
Both
-srand-srdare not set:- No HTTP response will be saved.
OutputDirwill be set to its default value, and any other data that needs to be saved will be stored in this directory.
-
Only
-sris set:- The HTTP response, along with any other data, will be saved in the
OutputDir, which will be the default directory.
- The HTTP response, along with any other data, will be saved in the
-
Only
-srdis set:- Everything, including the HTTP response, will be saved to the
OutputDirprovided with the-srdflag.
- Everything, including the HTTP response, will be saved to the
-
Both
-srand-srdare set:- Everything will be saved to the
OutputDirprovided with the-srdflag.
- Everything will be saved to the
In the config struct, I have used the StoreResp boolean as the key indicator of whether to store the response. Therefore, if the user provides only the -srd flag, we will set StoreResp to true.
There was a problem hiding this comment.
As far as I can understand point 3 and 4 are the same right?
There was a problem hiding this comment.
Yes, just in point 3 we will set the StoreResp value to true
internal/file/file.go
Outdated
| if os.IsNotExist(err) { | ||
| if _, err := os.Stat("output-cariddi/"); os.IsNotExist(err) { | ||
| CreateOutputFolder() | ||
| if _, err := os.Stat(outputDir + "/"); os.IsNotExist(err) { |
There was a problem hiding this comment.
Maybe it's better to use Sprintf
internal/file/file.go
Outdated
| if os.IsNotExist(err) { | ||
| if _, err := os.Stat("output-cariddi/"); os.IsNotExist(err) { | ||
| CreateOutputFolder() | ||
| if _, err := os.Stat(outputDir + "/"); os.IsNotExist(err) { |
pkg/crawler/useragents.go
Outdated
| rng := rand.New(source) | ||
|
|
||
| decision := rng.Intn(100) | ||
| const maxRandomValue = 100 |
There was a problem hiding this comment.
Put all constants at the top of this file.
Like
const (
maxRandomValue = 100
)
pkg/output/output.go
Outdated
| "github.com/edoardottt/cariddi/pkg/scanner" | ||
| ) | ||
|
|
||
| // constant defined in file.go as well, redefining here for circular dependency. |
There was a problem hiding this comment.
Isn't possible to define just one constant somewhere (file / output) and import that?
There was a problem hiding this comment.
Updated the PR. This comment was incorrect, constant is defined just once so removed the comment.
|
any update on this review? |
|
Hi @edoardottt, yes I will update the PR by weekend. But do we agree on StoreResp part in the first review comment. |
yes, we do :) |
|
@edoardottt , PR is updated, incase you missed |
edoardottt
left a comment
edoardottt
left a comment
There was a problem hiding this comment.
On the syntax + coding best practices side t's okay.
I think we need more unit tests before an actual try. For example, does the changes handle weird (or even unacceptable) input for output folder?
Before the PR changes output-cariddi was used, so no problem on that side. What if I use !%&/&%$\"$%&&/%&(&(=7909 as input?
Oc this is just an example, but we need to understand what could be wrong.
Let me know your thoughts on this,
edoardo
|
yes I agree with you. Let me add some layer of validation and test cases accordingly. |
|
@edoardottt , update the PR based on review |
|
Hi @kartikeysemwal , both Go / build and golangci-lint / lint actions are failing |
|
@edoardottt, I have updated the PR. Apologies for the inconvenience. The build failures were specific to the Linux system, and I initially created the PR on a Windows machine. Unfortunately, my WSL is currently broken, so I wasn't able to verify the new commit locally. |
2 participants
Fixes #129
The code changes include new -srd flag. The directory will be used to save all the output files, such textOutput, htmlOutput, indexResponses and raw response. Earlier all these file used to be saved in the "output-cariddi" in the same directory.
Fixes #130
The output path of the stored response will also be printed in the json output if -json flag is enabled