Conversation
8669cca to
ee5c1cb
Compare
fblupi
left a comment
There was a problem hiding this comment.
It works, but I would like to have a second opinion about this change from someone who knows the product better than me. Could any of you @ahukkanen @andreslucena take an eye on this too?
ahukkanen
left a comment
There was a problem hiding this comment.
Tested it and it works for the specified use cases.
Only thing I would suggest is to move this to the decidim-verifications module as I understand this is related to the verifications? At least this is where I tested it.
I also understand this can be hard to implement generically because different authorization methods can have different parameter names and there is currently no way knowing which of those parameters are sensitive and which are not.
Another thought I had is that Rails also adds this initializer automatically when the application is generated (take a look at the default initializers):
https://github.com/rails/rails/blob/de96c2e222fb3070422cbebbe039582256d58147/railties/lib/rails/generators/rails/app/templates/config/initializers/filter_parameter_logging.rb.tt#L5
It could be that they meant this to be something application specific but I can also understand why we want to add some defaults here as well. The list of defaults cannot be modified unless we change the whole initializer template.
|
We cannot actually move it to verifications, as one of fields that I am sanitizing is actually coming from initiative signatures (initiatives module). Also, considering this is something that impacts the entire application, i would keep it centralized inside of decidim-core. Considering that I have just filtered some of the fields that looked sensitive to me, I would consider that is not an exhaustive list of all the fields, therefore, we would need to have it in an initializer in the decidim core, so that we can actually filter "what is ours". Changing the application initializer automatically may lead to some other unforseen issues caused by additional security improvements done at the application level. |
ahukkanen
left a comment
There was a problem hiding this comment.
We cannot actually move it to verifications, as one of fields that I am sanitizing is actually coming from initiative signatures (initiatives module). Also, considering this is something that impacts the entire application, i would keep it centralized inside of decidim-core.
OK, fair and understandable reasoning for having this in the core. Affects multiple modules.
Considering that I have just filtered some of the fields that looked sensitive to me, I would consider that is not an exhaustive list of all the fields, therefore, we would need to have it in an initializer in the decidim core, so that we can actually filter "what is ours".
Yep but this does not cover everything even with the official modules shipped with releases. E.g. the postal letter verification has full_address which can be considered sensitive. Assembly members have birthday, birthplace and gender which can all be considered sensitive. The dummy authorization handler also has birthday which can be considered sensitive.
Not saying we need to add all of those but just emphasizing my point that this is hard to do generically for all possible use cases. Some may consider some data sensitive, some others may not.
Changing the application initializer automatically may lead to some other unforseen issues caused by additional security improvements done at the application level.
This I don't fully understand because I imagine most people are just going with the default initializers that are shipped with the generated application. This initializer is not automatically updated during Rails upgrades, so the security issues will be there anyways for most people if new filtered parameters are introduced in the default template shipped with Rails.
All in all, I'll approve this one as you reasoned it well why this needs to live in the core module. That was my only actual concern.
* Redesign: counteract the hover effects of the dropdown (decidim#11253) * hack to counteract the hover effects of the dropdown * keep menu unreachable onload * due to a bug in Chrome, move the inline events to script tags more info: https://bugs.chromium.org/p/chromium/issues/detail?id=868224&q=ontransitionend&colspe[%E2%80%A6]Block%20Component%20Status%20Owner%20Summary%20OS%20Modified * fix spelling * extract script to separate file * restore removed element * fix spelling * trigger manually dropdown controller on hover parent * move onmouseenter only to link * handle dropdowns only via css animation * no longer required * fix styles * disable animations on page load * fix tests * fix syntax * Filter parameters details (decidim#11418) * Tiny typo fix (decidim#11428) * Redesign: rename tests (decidim#11184) * Remove skip_unless_redesing_enabled calls in tests * Remove redesign_helpers for tests * Unskip and adapt REDESIGN_PENDING examples * Remove deprecated examples * Allow to configure the default header level of attributes titles in diff cell * Ensure accessibility of h in collaborative draft show * Recover accessibility tests in proposals * Remove comments * Unskip and adapt test * Fix expected text of link in test * Remove check for removed follow actions in participatory text index * Unskip tests * Unskip and adapt to vote proposal tests * Remove deprecated methods * Remove deprecations in tests * Remove collaborative drafts and proposals deprecated m cells * Adapt proposal m cell cache hash to l cell * Adapt proposal m-cell specs to l-cell * Replace use of proposal_m card * Remove condition on redesign from test * Rename shared examples for attachments to not use redesign term * Remove tests for CTA behavior deprecated in processes card-g * Avoid duplicated ids of highlighted participatory process in menu breadcrumb * Recover skipped examples * Recover accessibility tests in participatory processes * Remove participatory process description specs and include expected contents with metadata content block enabled * Unskip REDESIGN_PENDING tests * Move common statistics cells tests to shared examples * Remove unused process metadata item * Remove deprecated proceses m card and add tests for l and g cards * Remove deprecated process group m card cell * Recover tests after redesign in initiatives * Remove skipped deprecated tests * Remove assemblies description page test and move examples to show page with appropriate content blocks * Remove assembly m card and related tests * Unskip tests after completing redesing * Adapt sorting test to redesigned interface * Remove deprecated sorting tests because the redesigned interface prevents incomplete sortings * Unskip tests * Remove post m card cell * Remove reference to redesign from test * Remove comment m card * Unskip accessibility test in debates versions * Remove deprecated test * Remove deprecated comments about redesign * Remove deprecated test The proposals card-l does not show the supports count in the metadata, so there is no difference when supports are enabled on proposals component or not * Remove useless reference to redesign in test * Remove REDESIGN_PENDING comment: do not use card-l cell for media links * Remove REDESIGN_PENDING comments * Remove unused partial * Unskip tests * Remove conference m card cell * Adapt authorizations tests to redesign * Remove deprecated tests * Avoid errors in upload modal when file validation fails * Unskip tests * Remove deprecated test * Remove REDESIGN_PENDING comments * Update map styles in proposals * Uncomment tests * Uncomment and adapt selector in tests * Remove reference to redesign * Fix linter offense * Remove obsolete brakeman skip and update other in proposals * Remove unused translations and add some wizards keys to ignore unused * Update expected url in activity cell with comments activities * Increase header levels in version cell to include h1 * Recover missing data attribute in hero content block * Fix coordinates update in add_proposal js * Hide map only if there is no address with coordinates * Empty address field before changing it in test * Add issue reference to skipped tests * Display replies on deleted comments and keep then with AJAX deletion refresh * Unskip test * Update decidim-core/app/cells/decidim/version/show.erb Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro> * Use h2 tag * Move shared examples require to core * Replace actions with method doing the same * Fix test * Remove references to redesign in shared examples --------- Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro> * Redesign: refactor the scope of the main Javascript initializer (decidim#11317) * set the scope of all functions that are initialized at startup * scope the datepicker as well * remove precated js stuff (testing) * fix lint & place accordingly the initializations * Redesign: deprecate foundation-datepicker from public site (decidim#11420) * deprecate foundation-datepicker public site * fix core tests * fix datepicker tests * fix lint and rubocop * replace now with current * fix tests * fix more tests * fix tests * remove byebugs * Redesign: don't submit forms on modal's cancel * Update documentation for here map configuration (decidim#11447) * Two-columns layout fix (decidim#11400) * Column conversations fix * suggested changes added to &__aside * &__main updated * Changes made to span on &__main * test * spacing cli test * Disabler for pointer events (decidim#11449) * Added a disabler for pointer event * Improved workaround for the tabs +1 * Indentation line 19 * Fix test component (decidim#11436) * Fix label position on report button (decidim#11435) * Fix pipeline after decidim#11449 (decidim#11455) * Upgrade postgres from 11 to 14 (decidim#11453) * Fix initiatives ransack filters (decidim#11329) * Revert "Lock ChromeDriver to the latest working version (decidim#11391)" (decidim#11458) This reverts commit 2450714. * Force capital starting letter for PR titles (decidim#11457) * Force capital starting letter for PR titles * Force the starting character to be capitalized * Allow disabling password expiration (decidim#11135) * Allow disabling password expiration NIST does not recommend expiring passwords: https://pages.nist.gov/800-63-FAQ/#q-b05 * Add documentation about the new option * Fix linter * Fix linter * Fix autocomplete result list duplicates (decidim#11442) * Fix autocomplete user group duplication / Add system test * Fix test * Redesign: organization colors (decidim#11451) * refactor the organization colors * typo * remove specs about them_color input * fix specs * use old red for testing * set the default redesign colors to the organization factory (color-contrast) * replace the uses of legacy primary color * Redesign: axe accessibility issues (decidim#11403) * dropdown card does not require an id * add aria-label to progress bar * disable dropdown for desktop navigation * Refactor the use of filter_text_for to be in views instead of helpers * Allow passing an id to filter_text_for method * remove element_id (testing) * pass the id to the label * distinguish filters and order controls * distinguish all uses of dropdown-menus * rename control * don't use a list when you need divs * disable dropdown for desktop navigation * update code for emoji * remove ids from tests * fix core tests * fix election tests * fix tests * remove profile__user-role class * remove needless aria-attribute * remove duplicated ids * empty merge * Include alternative title for images when there's nothing but the title * disable dropdown for desktop * add alt text to default image * user profile must not be an h5 * duplicate_id and copy button SR text * fix odd layout for edit initiative * redesign committee requests form * fix a11y issue on budgets project list * mark layout item header al role=complementary since it's outside * do not expand elements if the page has very few contents * fix a11y issue on debates, sortitions and forms * fix rubocop * fix erblint * use the units as the aria-label * standarize arias for progress bars * initiatives style glitch --------- Co-authored-by: Eduardo Martinez Echevarria <eduardomech@gmail.com> * Integrate Bullet in decidim-dev (decidim#11416) * Initial commit on bullet integration * remove uneeded condition * Disable failing actions * Disable failing workflows * Disable all the workflows * Refactor actions * Revert generator change * Redesign: comment button (decidim#11461) * render the comment button depending of the settings * enhance condition: initiatives is not a component * handle comments_blocked? in case of null * fix offenses * refactor duplicated views (decidim#11459) * Redesign: remove the item itself from nav menu (decidim#11452) * remove the item itself from nav menu * fix i18n * fix specs * fix specs * Refactor Decidim jobs to inherit from Decidim::ApplicationJob (decidim#11468) * overwrite dropdown defaults for tablet size (decidim#11479) * Fix ambiguous id column on projects query (decidim#11474) * Redesign: resource metadata (decidim#11463) * make every resource metadata equal * remove any reference to "drawer" * typo * slightly different initiative layout * Redesign: display alert when no projects (decidim#11480) * display alert when no projects * normalize locals * Redesign: system (decidim#11478) * prepare assets and login page * responsive sidebar system * organization forms * refactor organization forms * password forms * admins forms * oauth forms * fix typo * fix specs and required update modal * fix lint * fix lint * update test settings * avoid aside to expand * remove deprecated styles * Fix persistance of the area in initiative creation (decidim#11476) * Lock TipTap editor to ~2.0.3 (decidim#11492) * Redesign: consolidate 2-col layout (decidim#11426) * spacing for participatory_processes, proposals, collaborative drafts, participatory texts and component announcement * spacing for sortitions, elections, meetings and fixes over proposals * spacing for assemblies and fixes over participatory spaces * fix tests * spacing for core * spacing for conferences, blogs and accountability. Fixes over conversations * aside heading responsive and spacig for budgets * spacing for projects, debates and initiatives * fix initiative specs * fix proposals specs * fix specs * missing space for collaborative drafts * avoid overwritting css rule du to tailwind directive * missing class * avoid weird space around the span * missing condition * Encode non-ASCII characters on external links (decidim#11472) * Encode non-ASCII characters on external links * Lint * Apply suggestions * Fix weird behavior with votes in comments (decidim#11490) * Fix weird behavior with votes in comments * Fix flaky test * Update remixicon to 3.5.0 (decidim#11494) * Replace close text with icon in order to enlarge it (decidim#11487) * Remove deprecated views & translations (decidim#11488) * Incorrect component link generation (decidim#11432) * Fix SEO routing issue for components index * Add tests for the redirects * Fix failing tests * Fix failing specs * Fix all failing tests * Fix linting error * default gray (non-text contrast) (decidim#11502) * Highlight the toggle contrast (decidim#11506) * Allow tabbing on mobile devices (decidim#11501) * Redesign: finish create-initiative step (decidim#11493) * simplify the finish create-initiative step * restore original condition * Fix the length of edit button in proposals (decidim#11497) * Fix wrong 'no activity' text on last activities page (decidim#11496) * Fix wrong 'no activity' text on last activities page * Fix failing spec * Redesign: meetings code icon (decidim#11486) * replace qr code icon with another related * change icon * Redesign: highlight links on hover (decidim#11508) * hightlight links on hover * highlight footer links * distinguisable link banner * place "skip to content" the first thing of all * Focus styles (decidim#11509) * Redesign: semantic datetimes (decidim#11513) * include datetime semantically * calendar a11y * ignore missing translation * Make visible the skip map option (decidim#11515) * Render partner without any link if it's not configured (decidim#11512) * Show different text for the Register CTA when the user is already registered (decidim#11511) * Show different text for the Register CTA when the user is already registered * Change two more buttons * Normalize i18n * Don't show initiative state field instead of showing it disabled (decidim#11518) * Don't show initiative state field instead of showing it disabled * Don't show the field in the edit form neither * Add tests * Lint files * Update decidim-initiatives/app/views/decidim/initiatives/initiatives/_form.html.erb Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro> --------- Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro> * Display special characters in breadcrumb menu (decidim#11514) --------- Co-authored-by: Hugoren Martinako <aumpfbahn@gmail.com> Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro> Co-authored-by: Ruth Cheesley <ruth@ruthcheesley.co.uk> Co-authored-by: Eduardo Martínez <eduardomech@gmail.com> Co-authored-by: Sina Eftekhar <104360479+sinaeftekhar@users.noreply.github.com> Co-authored-by: Tom <101816158+greenwoodt@users.noreply.github.com> Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi> Co-authored-by: Carlo Beltrame <carlo@beltra.me> Co-authored-by: JoonasAapro <110532525+JoonasAapro@users.noreply.github.com> Co-authored-by: Fran Bolívar <francisco.bolivar@nazaries.com>
🎩 What? Why?
While investigating #10731 i have notices there are some sensitive personal data that may leak into logging system.
📌 Related Issues
Link your PR to an issue
Testing
📷 Screenshots
Please add screenshots of the changes you are proposing
