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

Cache Key Validation #4637

Merged
merged 3 commits into from
May 5, 2021
Merged

Cache Key Validation #4637

merged 3 commits into from
May 5, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 2, 2021

Description
Cache keys are currently a free-for-all, though the specification is much tighter than that. Additionally, some handlers have limits on the length of key that can be used. So far the only one I know for sure is FileHandler, because using too long of a key fails to create the file since it is invalid in the filesystem.

This PR adds cache key validation and hashing to address the above issues. Note that I am not a cache expert, so if someone with more knowledge or experience wants to chime in I would be grateful. For example, the restricted characters were based on PSR-6 but some handlers may actually allow broader key usage than that.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner MGatner mentioned this pull request May 2, 2021
4 tasks
@MGatner MGatner requested a review from paulbalandan May 2, 2021 17:11
@MGatner
Copy link
Member Author

MGatner commented May 3, 2021

Thanks for the review @paulbalandan. I'd like your feedback on my responses before I implement any changes.

@MGatner
Copy link
Member Author

MGatner commented May 3, 2021

@paulbalandan Go to my fork and select the psr-cache branch and that will give you an idea of what I'm working towards. This is part of a much bigger project to supply adapters for most PSR components that we don't supply natively. In this case, I am working on a project at my day job that uses a package which requires a PSR-6 implementation; currently I have to include an external cache library to make this work, even though we have a very healthy cache layer.

system/Cache/Handlers/BaseHandler.php Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
tests/system/Cache/Handlers/BaseHandlerTest.php Outdated Show resolved Hide resolved
system/Cache/Handlers/FileHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
system/Cache/Handlers/WincacheHandler.php Outdated Show resolved Hide resolved
/**
* Maximum key length.
*/
public const MAX_KEY_LENGTH = PHP_INT_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

PHP_INT_MAX is an excessively long amount. Can we have a reasonable number here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's basically meant to be "unlimited". I couldn't find limits for all the handlers, but P/Redis stores the keys as binary strings with a limit of 512 MB - huge, in other words: https://stackoverflow.com/questions/5606106/what-is-the-maximum-value-size-you-can-store-in-redis

I'm open to other suggestions, or varying the specific handlers more, but that's how I got there.

/**
* Reserved characters that cannot be used in a key or tag.
*
* @see https://github.com/symfony/cache-contracts/blob/c0446463729b89dd4fa62e9aeecc80287323615d/ItemInterface.php#L43
Copy link
Member

Choose a reason for hiding this comment

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

While I do not have a problem with proper code attribution, should this @see be addressed to the PSR-6 document instead? This gives me a false impression that Symfony Cache is the authoritative source for the list of reserved cache characters whereas PSR-6 clearly gives this out.

Key - A string of at least one character that uniquely identifies a cached item. Implementing libraries MUST support keys consisting of the characters A-Z, a-z, 0-9, _, and . in any order in UTF-8 encoding and a length of up to 64 characters. Implementing libraries MAY support additional characters and encodings or longer lengths, but must support at least that minimum. Libraries are responsible for their own escaping of key strings as appropriate, but MUST be able to return the original unmodified key string. The following characters are reserved for future extensions and MUST NOT be supported by implementing libraries: {}()/@:

Copy link
Member

Choose a reason for hiding this comment

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

Just read this line: but MUST be able to return the original unmodified key string. Since MD5 hashing is a one-way process, then aren't we deviating again from PSR-6?

Copy link
Member Author

Choose a reason for hiding this comment

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

That link is because I copied this code from Symfony, more of an attribution than a reference. Maybe I should remove the @see and just have the link?

BaseHandler::validateKey() is not trying to be PSR-6 compliant - we'd have bigger issues to address if that were the case. The PSR-6 adapter will use this but handle its own keys and exceptions.

@MGatner
Copy link
Member Author

MGatner commented May 5, 2021

Test failures are unrelated:

55 Err:20 https://packages.microsoft.com/ubuntu/20.04/prod focal InRelease
56   Could not connect to packages.microsoft.com:443 (13.90.56.68), connection timed out [IP: 13.90.56.68 443]
57 Err:21 https://packages.microsoft.com/repos/azure-cli focal Release
58   Unable to connect to packages.microsoft.com:https: [IP: 13.90.56.68 443]
59 Reading package lists...
60 E: The repository 'https://packages.microsoft.com/repos/azure-cli focal Release' does not have a Release file.
61 Error: Process completed with exit code 100.

Any more thoughts on this one?

@MGatner MGatner requested a review from paulbalandan May 5, 2021 15:12
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I'm good with this, unless Lonnie has some more to add. 😁

@MGatner MGatner merged commit 2e0e020 into codeigniter4:develop May 5, 2021
@MGatner MGatner deleted the cache-hash branch May 5, 2021 18:45
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.

3 participants