Skip to content
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

enhancement: Gitleaks schema improvements #3675

Merged
merged 6 commits into from
Jun 22, 2024

Conversation

TommyE123
Copy link
Contributor

GitLeaks Add missing schema properties

Fixes #3064

Context:

Three schema references were missing for Gitleaks which causes YAML_V8R to fail.
REPOSITORY_GITLEAKS_PR_COMMITS_SCAN
REPOSITORY_GITLEAKS_PR_SOURCE_SHA
REPOSITORY_GITLEAKS_PR_TARGET_SHA

Testing:

Shows they were missing from the schema

image
image

If there are any issues or concerns, please let me know, and I will address them promptly.
Thank you for reviewing this PR and I look forward to your feedback.
Tom

@TommyE123 TommyE123 changed the title Gitleaks schema improvements enhancement: Gitleaks schema improvements Jun 19, 2024
@echoix
Copy link
Collaborator

echoix commented Jun 19, 2024

Were they added manually? Usually they are generated by a script, build.sh --doc. If this doesn't regenerate correctly, it's another issue.

@TommyE123
Copy link
Contributor Author

Yes I added them manually.

I was just going off what @nvuillam said in the linked issue that was raised a while back!

@echoix
Copy link
Collaborator

echoix commented Jun 19, 2024

The worse is that at some point when the command will be rerun it will get replaced by the actual contents from the descriptor.

@TommyE123
Copy link
Contributor Author

Apologies @echoix,

I should have checked in more detail and wasn’t aware the schema was created off the back of the descriptors as well.
I’d like to try and do this properly if that’s ok?

Tom

@TommyE123 TommyE123 marked this pull request as draft June 20, 2024 08:32
@echoix
Copy link
Collaborator

echoix commented Jun 20, 2024

You can run in your branch to see if the contents are different. Maybe it's the same? Maybe the full doc rebuild is done near releases, were these fields new?

@TommyE123
Copy link
Contributor Author

Hi @echoix,

These variables are from a previous release and don't get added via the build.sh --doc command, which is why they're missing from the JSON schema. After looking at the .automation/build.py code, it seems like there's currently no support for automatically adding these types of variables to the schema.

While I considered trying to add that functionality, it would require careful planning and testing to ensure it properly handles different variable types and doesn't cause any unintended issues or conflicts with existing variables.

For now, I've reordered these missing variables to match the order that the build.sh --doc command would place them in. This should work until we can come up with a more permanent solution.

Does this approach sound reasonable? I'm open to any other suggestions you might have for dealing with these missed variables and keeping the schema in sync going forward.

Tom

@TommyE123 TommyE123 marked this pull request as ready for review June 20, 2024 11:59
@echoix
Copy link
Collaborator

echoix commented Jun 20, 2024

Yes good

@echoix
Copy link
Collaborator

echoix commented Jun 21, 2024

Hi @echoix,

These variables are from a previous release and don't get added via the build.sh --doc command, which is why they're missing from the JSON schema. After looking at the .automation/build.py code, it seems like there's currently no support for automatically adding these types of variables to the schema.

While I considered trying to add that functionality, it would require careful planning and testing to ensure it properly handles different variable types and doesn't cause any unintended issues or conflicts with existing variables.

For now, I've reordered these missing variables to match the order that the build.sh --doc command would place them in. This should work until we can come up with a more permanent solution.

Does this approach sound reasonable? I'm open to any other suggestions you might have for dealing with these missed variables and keeping the schema in sync going forward.

Tom

It would be something very useful to have implemented in the docs generation, jsonschema docs generation or even normal build, as updating them manually is error prone (ie: forget to do it), as you found out recently.

I'm grateful for your work, but I'm blocked with the current CI that fails and can't fix them all at once. Nicolas would be able to force the merge of these PRs, but I can't just by myself.

@echoix
Copy link
Collaborator

echoix commented Jun 22, 2024

I’ve been merging your PRs one by one, resolving the conflicts on the changelog as we waited too long to merge.
Again, thanks for your great work!

@echoix echoix merged commit 94438f9 into oxsecurity:main Jun 22, 2024
6 checks passed
@nvuillam
Copy link
Member

Indeed, thanks @TommyE123 for your great latest contributions, and thanks @echoix for all your validation & guidance work ❤️

@TommyE123 TommyE123 deleted the gitleaks_schema_Improvements branch June 23, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPOSITORY_GITLEAKS_PR_COMMITS_SCAN missing from the configuration schme
3 participants