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

AssetBuilder - Fix testInvalid() failure. Switch to JWT. #25305

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

totten
Copy link
Member

@totten totten commented Jan 10, 2023

Overview

This fixes a test-failure that recently appeared in AssetBuilder. Additionally, it switches the parameter-validation to JWT (for better security checks). For sites that lack the JWT pre-requisites (ie CIVICRM_SIGN_KEYS), it provides a transitional key.

Before+After

Based on the asset parameters, there is a digest-code. The usage of this digest-code evolves as follows:

When Description
Before
(Circa 5.56.0)
Digests are used for file-names and caching. Parameters (for the on-demand/debug variants of assets) are not validated.
Before
(Circa 5.57.0)
Digests are used for file-names, caching, and parameter validation.
After
(This Patch)
Digests are used for file-names and caching. JWT does parameter validation.

Technical Details: Background

AssetBuilder was recently (5.57.0) updated to validate the digest-code in asset URLs (for the on-demand/debug variants of assets). But this led to new failures in E2E\Core\AssetBuilderTest. Why?

The digest code relies on a semi-secret value, the runtime ID. In this specific test (E2E\Core\AssetBuilderTest::testInvalid()), there are two processes. The PHPUnit process generates one digest-code (with one runtime ID) and sends it to the HTTP server -- but the HTTP server expects a different digest-code (with a different runtime ID).

Is it good or bad that phpunit+httpd have different runtime IDs? 🤷 Is it good or bad to use runtime ID here? I think runtime ID is fair as part of the cache identifier. But I don't think it's ideal for validating the origin of parameters. We have another tool (JWT) which is better designed for that.

Technical Details: This Patch

This patch restores the use of digest code (and runtime ID) -- they are only used for purposes of file-names and caching. They no longer influence the validation-check. Instead, we use JWT.

There is one catch about JWT, -- as observed in the discussion around #25285, JWT requires a signing-key, and some upgraded sites may not have the signing-key configured. Given the centrality of asset-builder in the upgrade-workflow (e.g. for civicrm/a/#/status), we need a fallback so that site-builders can apply the upgrade and do the configuration. This defines a very limited fallback.

@civibot
Copy link

civibot bot commented Jan 10, 2023

(Standard links)

@civibot civibot bot added the 5.58 label Jan 10, 2023
@totten totten force-pushed the 5.58-assetbuilder-2 branch from a299cd0 to 5f60c72 Compare January 10, 2023 21:28
@totten
Copy link
Member Author

totten commented Jan 11, 2023

civibot, test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001 seamuslee001 merged commit 1210f22 into civicrm:5.58 Jan 11, 2023
@totten totten deleted the 5.58-assetbuilder-2 branch January 11, 2023 19:56
olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Apr 12, 2023
olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants