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

Flag ".." in <file_name> as error condition #146

Merged
merged 2 commits into from
Apr 6, 2023
Merged

Flag ".." in <file_name> as error condition #146

merged 2 commits into from
Apr 6, 2023

Conversation

nutjob4life
Copy link
Member

🗒️ Summary

Merge this to have pds-deep-archive raise an exception when encountering <file_name> entries that contain relative paths, such as ../catalog/dataset.cat, which according to this comment is invalid in a PDS label.

⚙️ Test Data and/or Report

Before:

$ .venv/bin/pds-deep-archive --debug --site PDS_ATM --bundle-base-url https://pds-atmospheres.nmsu.edu/PDS/data/ /Users/kelly/Downloads/vo/vo_3002/bundle_voirtm.xml
INFO 👟 PDS Deep Archive, version 1.1.2
…
INFO 🎉 Success! From /Users/kelly/Downloads/vo/vo_3002/bundle_voirtm.xml, generated these output files:
INFO 📄 SIP Manifest: vo_irtm_v1.0_20230404_sip_v1.0.tab
INFO 📄 XML label for the SIP: vo_irtm_v1.0_20230404_sip_v1.0.xml
INFO 👋 That's it for now. Bye.

After:

$ .venv/bin/pds-deep-archive --debug --site PDS_ATM --bundle-base-url https://pds-atmospheres.nmsu.edu/PDS/data/ /Users/kelly/Downloads/vo/vo_3002/bundle_voirtm.xml
INFO 👟 PDS Deep Archive, version 1.1.3
…
CRITICAL 🛑 Cannot proceed as a critical problem has occurred; re-run with --debug for more info.
DEBUG 🖥 Here is the exception: ValueError('Bundle /Users/kelly/Downloads/vo/vo_3002/document/dataset.xml contains a <file_name> ``../catalog/dataset.cat`` which contains a relative path ``..``, which is invalid')
…

♻️ Related Issues

@nutjob4life nutjob4life changed the title I145 Flag ".." in <file_name> as error condition Apr 6, 2023
@nutjob4life nutjob4life requested a review from a team April 6, 2023 13:31
Copy link

@alexdunnjpl alexdunnjpl left a comment

Choose a reason for hiding this comment

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

v. nice

@nutjob4life nutjob4life merged commit b66fca0 into main Apr 6, 2023
@nutjob4life nutjob4life deleted the i145 branch April 6, 2023 16:36
@nutjob4life
Copy link
Member Author

Thanks @alexdunnjpl @c-suh! 🎉

@c-suh
Copy link

c-suh commented Apr 6, 2023

@nutjob4life thank you for the quick turnaround! Much appreciated.

@jordanpadams
Copy link
Member

@nutjob4life question: does deep-archive provide any support for directory_path_name? according to the spec, this is how someone could specify a path to a file not in the current directory.

@alexdunnjpl
Copy link

@jordanpadams that attribute doesn't explicitly allow for relative paths either (as it also requires a root/cwd to be meaningful, which isn't mentioned)... was it definitely intended to support relpaths?

@nutjob4life
Copy link
Member Author

@jordanpadams there is support for directory_path_name; see:

# any sibling directory_path_name?
dpnnode = match.getparent().find(f"./{{{PDS_NS_URI}}}directory_path_name")
fn = match.text.strip()
dn = None if dpnnode is None else dpnnode.text.strip()
filepath = os.path.join(dirpath, dn, fn) if dn else os.path.join(dirpath, fn)

@jordanpadams
Copy link
Member

@alexdunnjpl I was thinking the same thing. you can start getting really weird with things like ../../../../../path/to/file.dat. this may require an update/clarification to the standard

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.

4 participants