Skip to content

fix bug in output filename calculation in plot_radial_build.py#3008

Merged
timothy-nunn merged 1 commit intoukaea:mainfrom
alekitto:fix/plot_radial_build_filename
Dec 20, 2023
Merged

fix bug in output filename calculation in plot_radial_build.py#3008
timothy-nunn merged 1 commit intoukaea:mainfrom
alekitto:fix/plot_radial_build_filename

Conversation

@alekitto
Copy link
Copy Markdown
Contributor

@alekitto alekitto commented Dec 5, 2023

Description

Filename calculation in plot_radial_build.py is done taking the last 10 characters of the input file path.
This behaviour is not safe and can lead to NotFoundError to be thrown or cause path traversal in the worst case.

The fix replaces the unsafe part picking up the basename of the file (without extension) and prepending it to _radial_build.{save_format} static part.

This also fixes test_plot_radial_build run is some environments.

Checklist

I confirm that I have completed the following checks:

  • I have justified any large differences in the regression tests caused by this pull request in the comments.
  • I have added new tests where appropriate for the changes I have made.
  • If I have had to change any existing unit or integration tests, I have justified this change in the pull request comments.
  • If I have made documentation changes, I have checked they render correctly.
  • I have added documentation for my change, if appropriate.

Copy link
Copy Markdown
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

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

Hi @alekitto,

Thank you for your contribution to the PROCESS repository by identifying and proposing a fix to this bug.

I think pathlib may offer a nicer, more modern fix to this bug, and have suggested as such.

Comment thread process/io/plot_radial_build.py Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
f"{args.outputdir}/{os.path.splitext(os.path.basename(args.input))[0]}_radial_build.{save_format}",
f"{args.outputdir}/{Path(args.input).stem}_radial_build.{save_format}",

This is probably the correct and less-verbose way to do this in newer versions of Python (of course this would also make the import os redundant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. Fixed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you very much :)

@alekitto alekitto force-pushed the fix/plot_radial_build_filename branch from bf3029b to 1abed81 Compare December 11, 2023 09:04
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