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

Allow custom JSON files #2

Merged
merged 11 commits into from
Feb 29, 2024
Merged

Allow custom JSON files #2

merged 11 commits into from
Feb 29, 2024

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Feb 29, 2024

Packages like paws use subdirectories. These cases are extremely rare, so as we discussed in r-multiverse/help#1, this PR allows custom JSON entries for the little text files contributed to https://github.com/r-releases/r-releases. These are all detected as JSON, flagged for manual review, and merged into the universe.

@shikokuchuo, would you please review?

@wlandau wlandau self-assigned this Feb 29, 2024
@shikokuchuo
Copy link
Member

If I'm reading this correctly it allows any additional JSON fields, but I think we should specifically just check for 'subdir'. This will prevent simple exploits that try to add arbitrary data to our files.

@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

If I'm reading this correctly it allows any additional JSON fields, but I think we should specifically just check for 'subdir'. This will prevent simple exploits that try to add arbitrary data to our files.

build_universe() does allow custom JSON, but assert_package() flags all custom JSON for manual review: https://github.com/wlandau/r.releases.utils/blob/e3c024ffb6593731b2f2308f6bb248b52b25d60e/R/assert_package.R#L15-L24. Does the obligatory manual review address the exploit issue? I'm a little concerned that just focusing on subdir will prevent us from allowing custom JSON for other reasons we can't foresee yet.

@shikokuchuo
Copy link
Member

I would prefer to take the opposite approach as currently we only know about 'subdir'. It seems risky to accommodate an 'unkown unkown' as the manual review can still be overridden. Does that make sense?

@wlandau
Copy link
Member Author

wlandau commented Feb 29, 2024

Yeah, if the human review is careless, we would want another guardrail to prevent something truly calamitous. Added in 00cb225.

Copy link
Member

@shikokuchuo shikokuchuo left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple things.

R/assert_package.R Outdated Show resolved Hide resolved
R/build_universe.R Show resolved Hide resolved
R/build_universe.R Show resolved Hide resolved
@wlandau wlandau merged commit c4c76c0 into r-multiverse:main Feb 29, 2024
2 checks passed
@wlandau wlandau deleted the json branch February 29, 2024 16:20
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.

2 participants