-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
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
base: main
Are you sure you want to change the base?
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 |
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.
first review
@@ -90,6 +90,12 @@ func main() { | |||
StoreResp: flags.StoreResp, | |||
} | |||
|
|||
config.OutputDir = output.CariddiOutputFolder | |||
if flags.StoredRespDir != "" { |
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 should perform some tests. What if config.StoreResp is not specified?
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.
With the two tags -sr
and -srd
, there are four possible scenarios:
-
Both
-sr
and-srd
are not set:- No HTTP response will be saved.
OutputDir
will be set to its default value, and any other data that needs to be saved will be stored in this directory.
-
Only
-sr
is 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
-srd
is set:- Everything, including the HTTP response, will be saved to the
OutputDir
provided with the-srd
flag.
- Everything, including the HTTP response, will be saved to the
-
Both
-sr
and-srd
are set:- Everything will be saved to the
OutputDir
provided with the-srd
flag.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can understand point 3 and 4 are the same right?
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.
Yes, just in point 3 we will set the StoreResp value to true
internal/file/file.go
Outdated
} | ||
|
||
_, err := os.Stat(filename) | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to use Sprintf
internal/file/file.go
Outdated
_, err := os.Stat(filename) | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sprintf
pkg/crawler/useragents.go
Outdated
@@ -151,7 +151,8 @@ func GenerateRandomUserAgent() string { | |||
source := rand.NewSource(time.Now().UnixNano()) | |||
rng := rand.New(source) | |||
|
|||
decision := rng.Intn(100) | |||
const maxRandomValue = 100 |
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.
Put all constants at the top of this file.
Like
const (
maxRandomValue = 100
)
pkg/output/output.go
Outdated
@@ -36,6 +36,7 @@ import ( | |||
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't possible to define just one constant somewhere (file / output) and import that?
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.
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 |
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.
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. |
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