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.

Sign up for GitHub

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

Jump to bottom

More secure path mangling #281

Merged
merged 1 commit into from
Dec 7, 2022
Merged

More secure path mangling #281

merged 1 commit into from
Dec 7, 2022

Conversation

marcjansen
Copy link
Member

@marcjansen marcjansen commented Nov 26, 2021 •
edited
Loading

This is a follow up from the excellent #280. I think the security might even be improved by the following PR.

I'd really like a review from @anikaweinmann or @mmacata on this; this is open for discussion.

@marcjansen
Copy link
Member Author

Hmm. Probably some occasions missed. Hopefully just that.

@marcjansen marcjansen force-pushed the more_secure_paths branch 2 times, most recently from 07d756e to dfa4622 Compare November 26, 2021 21:37
anikaweinmann
anikaweinmann reviewed Nov 29, 2021
View reviewed changes
src/actinia_core/core/utils.py Show resolved Hide resolved
src/actinia_core/core/utils.py Outdated Show resolved Hide resolved
@marcjansen marcjansen marked this pull request as ready for review December 13, 2021 19:46
@marcjansen
Copy link
Member Author

Please have another look @anikaweinmann I marked this as ready for review now. I added some arguments to the two lines you remarked on. I'd love to hear your thoughts again.

@anikaweinmann
Copy link
Member

@marcjansen do you want to add a r/w parameter to the functions? Otherwise it is also fine for me to merge it as it is.

@neteler neteler added this to the next_patch milestone Nov 8, 2022
@marcjansen
Copy link
Member Author

Sorry for not giving this more attention. I think the general idea is still valid, but I am not sure whether this catches all relevant cases.

I see this has an approving review, and I can try to rebase this, if you still want it. I can also live with you guys and girls closing this PR with unmerged commits. Please go ahead as you see fit.

@anikaweinmann
Copy link
Member

@marcjansen you are welcome to merge and rebase

@marcjansen marcjansen force-pushed the more_secure_paths branch 2 times, most recently from d8ea8e9 to fa2f7af Compare November 25, 2022 08:28
@marcjansen
Copy link
Member Author

@marcjansen you are welcome to merge and rebase

I have done that and also added support for intents (r, w or rw) when ensure_valid_path is called.

Please have another close look at what is now in this PR. Where the method is called, I have added an intent where I was more or less sure, but these shpould please be double checked by someone with more know-how of the actual intent.

@marcjansen
Copy link
Member Author

anybody @mundialis-dev ?

@marcjansen
Copy link
Member Author

Friendly ping @mmacata @anikaweinmann

@anikaweinmann
Copy link
Member

Thank you, for me it look good. Please merge.

@marcjansen
Copy link
Member Author

Thanks for the review.

@marcjansen marcjansen merged commit be5299e into actinia-org:main Dec 7, 2022
@marcjansen marcjansen deleted the more_secure_paths branch December 7, 2022 16:40
@anikaweinmann anikaweinmann modified the milestones: next_patch, 4.5.0 Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anikaweinmann anikaweinmann anikaweinmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@marcjansen @anikaweinmann @neteler