fix recursive struct validation during JSON marshaling #5883
Conversation
WalkthroughThe changes in this pull request focus on enhancing the JSON marshaling and unmarshaling processes for the Changes
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/templates/templates.go (3)
524-527: Consider validating the template before marshaling.Currently, validation occurs after marshaling. It's generally better to validate the
templatebefore attempting to marshal it. This ensures that only valid data is marshaled, and any validation errors are caught early.Apply this diff to perform validation prior to marshaling:
func (template *Template) MarshalJSON() ([]byte, error) { + errValidate := validate.New().Struct(template) + if errValidate != nil { + return nil, errValidate + } type TemplateAlias Template // avoid recursion out, marshalErr := json.Marshal((*TemplateAlias)(template)) - errValidate := validate.New().Struct(template) return out, marshalErr }
Line range hint
536-538: Use consistent type alias naming inUnmarshalJSON.In
UnmarshalJSON, consider renaming the type alias fromAliastoTemplateAliasfor consistency withMarshalJSON. This improves code readability and maintains a consistent coding style.Apply this diff to rename the type alias:
func (template *Template) UnmarshalJSON(data []byte) error { - type Alias Template - alias := &Alias{} + type TemplateAlias Template + alias := &TemplateAlias{} err := json.Unmarshal(data, alias) if err != nil { return err
Line range hint
549-551: Error message clarity inUnmarshalJSON.The error message when failing to unmarshal a multi-protocol template includes the template ID. Ensure that
template.IDis correctly populated at this point; otherwise, the error message may be less informative.If
template.IDmight not be set before this point, consider including more context or verifying that the ID is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/templates/templates.go(1 hunks)
🔇 Additional comments (2)
pkg/templates/templates.go (2)
524-525: Properly prevent recursion in MarshalJSON using a type alias.
The introduction of TemplateAlias as a type alias for Template effectively prevents infinite recursion during JSON marshaling. This is a correct implementation to avoid stack overflow errors when the struct implements MarshalJSON.
Line range hint 542-555: Verify the handling of multiple requests during unmarshaling.
When checking for multiple requests, the code unmarshals the JSON data into a tempMap to preserve the order of protocols and requests. Ensure that this approach correctly handles all possible cases and does not introduce any unintended side effects.
To verify, consider running tests with templates that have multiple requests of various protocols and confirm that the order is preserved and no data is lost or corrupted.
Proposed changes
Closes #5882
example.go:$ go run . {"id":"azure-takeover-detection","info":{"name":"Microsoft Azure Takeover Detection","author":["pdteam"],"tags":["dns","takeover","azure"],"description":"Microsoft Azure is vulnerable to subdomain takeover attacks. Subdomain takeovers are a common, high-severity threat for organizations that regularly create and delete many resources. A subdomain takeover can occur when a D\u003e","reference":["https://godiego.co/posts/STO/","https://docs.microsoft.com/en-us/azure/security/fundamentals/subdomain-takeover","https://cystack.net/research/subdomain-takeover-chapter-two-azure-services/"],"severity":"high","metadata":{"max-request":1},"classification":{"cve-id":null,"cwe-id":["cwe-404"],"cvss-metrics":"CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:C/C:L/I:L/A:N","cvss-score":7.2}},"dns":[{"matchers":[{"type":"word","part":"body","words":["NXDOMAIN"]},{"type":"dsl","dsl":["contains(cname, \"azure-api.net\")","contains(cname, \"azure-mobile.net\")","contains(cname, \"azurecontainer.io\")","contains(cname, \"azurecr.io\")","contains(cname, \"azuredatalakestore.net\")","contains(cname, \"azureedge.net\")","contains(cname, \"azurefd.net\")","contains(cname, \"azurehdinsight.net\")","contains(cname, \"azurewebsites.net\")","contains(cname, \"azurewebsites.windows.net\")","contains(cname, \"blob.core.windows.net\")","contains(cname, \"cloudapp.azure.com\")","contains(cname, \"cloudapp.net\")","contains(cname, \"database.windows.net\")","contains(cname, \"redis.cache.windows.net\")","contains(cname, \"search.windows.net\")","contains(cname, \"servicebus.windows.net\")","contains(cname, \"trafficmanager.net\")","contains(cname, \"visualstudio.com\")"]}],"extractors":[{"type":"dsl","dsl":["cname"]}],"matchers-condition":"and","name":"{{FQDN}}","type":"A","retries":3,"attack":"","recursion":true}],"signature":"","variables":{}}Checklist
Summary by CodeRabbit
New Features
Bug Fixes