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

Base64 encode the scope value #51

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Base64 encode the scope value #51

merged 3 commits into from
Jan 12, 2024

Conversation

larsks
Copy link
Contributor

@larsks larsks commented Nov 22, 2023

bkt permits setting a cache scope (provided via the BKT_SCOPE
environment variable or the --scope command line option) to prevent
collisions between unrelated command invocations. Previously, the scope
value was used verbatim in a directory path, causing errors if the scope
contained path-sensitive characters such as /.

This commit modifies bkt to base64 encode the scope value. Given a scope
value like https://example.com/, that means we now end up with a path
like:

/tmp/bkt-0.7-cache/keys/aHR0cHM6Ly9leGFtcGxlLmNvbS8.C3823B193F1E0C78

Instead of the invalid:

/tmp/bkt-0.7-cache/keys/https://www.example.com/.C3823B193F1E0C78

Fixes #50

Copy link
Owner

@dimo414 dimo414 left a comment

Choose a reason for hiding this comment

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

Thank you! Welcome to Rust 😄

I'd appreciate if you could add test coverage as well; you should be able to duplicate this test with a scope that breaks prior to this PR.

src/lib.rs Outdated Show resolved Hide resolved
@larsks
Copy link
Contributor Author

larsks commented Nov 27, 2023

Thanks for the feedback. I'll try to take care of these sometime this week.

This commit adds a test that exercises the issue described in dimo414#50.
bkt permits setting a cache scope (provided via the `BKT_SCOPE`
environment variable or the `--scope` command line option) to prevent
collisions between unrelated command invocations. Previously, the scope
value was used verbatim in a directory path, causing errors if the scope
contained path-sensitive characters such as `/`.

This commit modifies bkt to base64 encode the scope value. Given a scope
value like `https://example.com/`, that means we now end up with a path
like:

    /tmp/bkt-0.7-cache/keys/aHR0cHM6Ly9leGFtcGxlLmNvbS8.C3823B193F1E0C78

Instead of the invalid:

    /tmp/bkt-0.7-cache/keys/https://www.example.com/.C3823B193F1E0C78

Fixes dimo414#50
@larsks
Copy link
Contributor Author

larsks commented Dec 5, 2023

@dimo414 just following up to see if this needs any more work. Thanks!

@dimo414
Copy link
Owner

dimo414 commented Dec 5, 2023

Sorry I had missed that you'd updated the PR (you can click the "re-request review" button to notify the reviewer - GitHub's review flow is pretty confusing 😕). Will take a look as soon as I can.

@larsks larsks requested a review from dimo414 January 10, 2024 19:56
src/lib.rs Outdated Show resolved Hide resolved
@dimo414
Copy link
Owner

dimo414 commented Jan 12, 2024

Thanks again! Will plan to cut a new release with this soon.

@dimo414 dimo414 merged commit 29ae490 into dimo414:master Jan 12, 2024
9 checks passed
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.

bkt should make scope "filename-safe"
2 participants