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

Sanitize content hashes in archive methods #175

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

Conversation

tillprochaska
Copy link
Contributor

@tillprochaska tillprochaska commented May 28, 2024

Our content hashes are always hex digests and we should reject anything that contains non-hex characters. This raises an exception when trying to call archive methods with invalid content hashes.

You can test this in Aleph by installing serivcelayer from this branch:

pip uninstall servicelayer
pip install "servicelayer[google,amazon] @ git+https://github.com/alephdata/servicelayer.git@fix/validate-content-hash"

Don’t forget to restart the api container after doing this :)

Our content hashes are always hex digests and we should reject anything that contains non-hex characters.
@tillprochaska tillprochaska force-pushed the fix/validate-content-hash branch from 07bea7b to e37a8dd Compare May 28, 2024 18:32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content hash also needes to be sanitized for the generate_url!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content hash needs to be sanitized for the generate_url method as well!

@tillprochaska
Copy link
Contributor Author

Probably need to also sanitize prefixes as well (e.g. in the path_prefix method).

Copy link
Contributor

@catileptic catileptic left a comment

Choose a reason for hiding this comment

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

Seems good to me. We can merge this after the remaining comments are addressed :)

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