Skip to content

caddy.go: Check whether @id is unique #7002

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chise1
Copy link

@Chise1 Chise1 commented May 7, 2025

Fixes: #6991

Check whether @id is unique.If it is not unique,rollback caddy config json.
I'm not sure if it can work properly under caddyfile.

@CLAassistant
Copy link

CLAassistant commented May 7, 2025

CLA assistant check
All committers have signed the CLA.

Comment on lines +227 to +230
err2 := rollbackRawCfg()
if err2 != nil {
err = errors.Join(err, err2)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to parse the old config here without checking the length of rawCfgJSON isn't a recommended approach. Additionally, this is a potential duplicate of the call in Ln246

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask if there are any areas in this pr that need to be modified and if it can be merged?

caddy.go Outdated
Comment on lines 268 to 269
err := json.Unmarshal(rawCfgJSON, &oldCfg)
rawCfg[rawConfigKey] = oldCfg

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obs: Potential nil assignment to rawCfg. Check for error before assigning to the map

@Chise1
Copy link
Author

Chise1 commented May 13, 2025

I added length detection and fixed a stupid problem. From the perspective of code analysis, "err" will not be repeated.

Changes of rewrite, involves nginx-adapter-case

@mohammed90
Copy link
Member

Changes of rewrite, involves nginx-adapter-case

Please keep the PR topical and focused. Split these changes into a separate PR.

@Chise1
Copy link
Author

Chise1 commented May 13, 2025

I have deleted the irrelevant pr

@mholt
Copy link
Member

mholt commented May 13, 2025

I feel like a better approach would be to check the ID as it's being read in and we're creating the mapping of IDs to their full config paths. If we return an error the rest should be handled automatically (no need to roll back, etc)

@Chise1
Copy link
Author

Chise1 commented May 14, 2025

I feel like a better approach would be to check the ID as it's being read in and we're creating the mapping of IDs to their full config paths. If we return an error the rest should be handled automatically (no need to roll back, etc)

If we do as you said, the content that needs to be modified is rather complicated. This is beyond my ability. I'm worried that the modification will introduce bugs.

@Chise1
Copy link
Author

Chise1 commented May 15, 2025

add one test.

@francislavoie francislavoie changed the title caddy.go: Check whether @id is unique(#6991) caddy.go: Check whether @id is unique May 15, 2025
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.

Add the uniqueness judgment of id
5 participants