CRM-20926 - Allow extensions to flag PHPIDS html/json fields #10709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
jenkins, test this please |
|
"Allow extensions to..." — This caught my eye. Would it be possible to add some documentation to the Developer Guide to go along with this PR so that extensions developers can learn how to do whatever it is that this PR would allow them to do? |
|
@seanmadsen This should be ready for you to give a whirl. I've posted some docs in civicrm/civicrm-dev-docs#227 along with an example of triggering the IDS in https://gist.github.com/totten/27cc1bde206fe17371c8a6d7cb361e49 |
totten
force-pushed
the
master-ids-cleanbreak
branch
from
48e6304
to
615e575
Compare
|
@seanmadsen Updates following our discussion
(The commit#'s have all changed following rebase, but the only new thing is 615e575.) |
|
I've reviewed this PR by looking over the code and running it locally.
Good to go! |
* This function does not appear to be called anywhere in `universe`. * It references the table/DAO for `civicrm_menu`, which seems like a bad idea. * This function goes back v3.0 (when the navbar and router were decoupled) -- but even then, it wasn't used! I'm guessing it was an interim and forgotten artifact written while decoupling navbar and router.
|
Thank you @seanmadsen! It was really helpful how your review involved trying it out -- that step found the I'll rebase to resolve the recent conflict and move the upgrade logic to match the new version#. Then it should be merge-on-pass. |
Background ========== We need a place to store per-page information about IDS configuration. More generally, as we continue adding features through extensions, we should anticipate that the route metadata will expand in ways that we can't anticipate. Adding a general-purpose serialized field will help us define new behaviors through extensions. Before ====== All metadata in the routing table (`civicrm_menu`) had to stored in a separate SQL column. After ===== Any unrecognized fields will be stored and loaded from the column `module_data`.
Background ========== PHPIDS scans all inputs and uses heuristic checks to identify malicious content. It relies on a configuration with data about the fields we'll process. Before ====== The function `createConfigFile()` produces the standard configuration as an INI file. The configuration was represented as an unalterable string. After ===== * The configuration is represented as an array. * The configuration is generated via `createStandardConfig()` * The function `createConfigFile()` only handles file I/O.
Background ========== Previously, the IDS configuration was written to a temp file and then read from the tempfile. Since we no longer read the tempfile, we don't need to write to it either. Before ====== System autocreates the `Config.IDS.ini` file. After ===== System does not create `Config.IDS.ini`.
The PHPIDS config format uses the property:
```
exceptions => array('field_1)`
```
But our XML format uses the tag:
```
<exception>field_1</exception>
```
Grammatically, those both make sense. But they were getting confused.
This modifies the XML parser to map the tags `<exception>` to property
`exceptions`, and it updates the unit-test to ensure that the data
(from the route and the default config) are correctly merged.
totten
force-pushed
the
master-ids-cleanbreak
branch
from
615e575
to
36d4fa1
Compare
|
@totten tests have passed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
PHPIDS scans all HTTP inputs and applies heuristics to identify malicious
submissions. It relies on a having configuration about the list of fields
we'll process.
NOTE: This PR includes/depends on #10708.
Before
The function
createConfigFile()produces the standard configuration as anINI file. The configuration was represented as an unalterable string.
After
createStandardConfig()Acceptance Criteria
civicase_reloadinvolved with CRM-20924 – can be flagged ashtml,json, or otherexceptions to the PHPIDS heuristics.