Page MenuHomePhabricator
Create Task
Maniphest T232820

Security Concept Review For client side error logging js client
Closed, ResolvedPublic
Actions

Description

Project Information

  • Name of project: client side error logging
  • Project home page: N/A
  • Name of team which owns the project: SRE / observability
  • Primary contact for the project: Filippo Giunchedi
  • Target date for deployment: N/A yet
  • Link to code repository: N/A yet
  • Is this a brand-new project: yes
  • Has this project ever been reviewed before: (Phab tasks, etc.) no
  • Has any risk assessment (STRIDE, etc.) been performed: not AFAIK
  • Is there an existing RFC or has this been presented to the community: no
  • Is this project tied to a team quarterly goal: not yet
  • Does this project require its own privacy policy: no

Description of the project and how it will be used

We are already performing some logging of client side errors (i.e. javascript errors from user agents) although with some limitations, the idea and scope of the project is to use lift those limitations and move to a supported system for all needs of client side error logging, see also T217142: [Proposal] Use the Kafka-Logstash logging infrastructure to log client-side errors. The security review involves the javascript client that will be used to send errors back to WMF, note the client is still being developed and available yet, I'm filing the review task in advance.

Description of any sensitive data to be collected or exposed

Collected: UA information, client IP, url that has errors. Exposed nothing, will be stored in logstash/kibana for 90d

Technologies employed

javascript

Dependencies and vendor code

N/A yet

Working test environment

N/A yet

Related Objects

Event Timeline

fgiunchedi created this task.Sep 13 2019, 10:23 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 13 2019, 10:23 AM
fgiunchedi mentioned this in T226986: Client side error logging production launch.Sep 13 2019, 10:36 AM
sbassett triaged this task as Lowest priority.Sep 17 2019, 4:53 PM
sbassett subscribed.Sep 24 2019, 4:49 PM

@fgiunchedi - Thanks for submitting this. We'll keep this in our Application Security Reviews backlog for the time being with lowest priority. If you could let us know as soon as you can (preferably 30 days out) when the code will be ready for review (ideally in a fairly finalized state with only minimal changes left before its first deploy) that would be great. Thanks.

fgiunchedi added a subscriber: Tgr.Sep 25 2019, 2:47 PM

Thanks @sbassett ! Will do, I'm cc'ing @Tgr too

sbassett added a comment.Dec 11 2019, 5:10 PM

@fgiunchedi - any update or progress on this? If not, we might just want to decline this task for now until there's actually some code ready for review. Thanks.

fgiunchedi added a comment.Dec 16 2019, 3:17 PM
In T232820#5732793, @sbassett wrote:

@fgiunchedi - any update or progress on this? If not, we might just want to decline this task for now until there's actually some code ready for review. Thanks.

Yes, progress in the form of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/553376 which is the client-side javascript part. The code is being reviewed and worked on although I don't think it is yet at the security-review stage. Having said that, feel free to close this task and we'll reopen/followup when needed.

sbassett changed the task status from Open to Stalled.Dec 16 2019, 4:22 PM
sbassett moved this task from Incoming to Frozen on the deprecated-security-team-reviews board.
• chasemp added a project: Application Security Reviews.Jan 7 2020, 3:38 PM
• chasemp removed a project: deprecated-security-team-reviews.Jan 7 2020, 3:47 PM
• chasemp moved this task from Incoming to Back Orders on the Application Security Reviews board.Jan 7 2020, 3:49 PM
• chasemp closed this task as Resolved.Jan 7 2020, 6:54 PM
• chasemp claimed this task.
• chasemp edited projects, added Security-Team, Security Preview; removed Application Security Reviews.
• chasemp subscribed.
In T232820#5744696, @fgiunchedi wrote:
In T232820#5732793, @sbassett wrote:

@fgiunchedi - any update or progress on this? If not, we might just want to decline this task for now until there's actually some code ready for review. Thanks.

Yes, progress in the form of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/553376 which is the client-side javascript part. The code is being reviewed and worked on although I don't think it is yet at the security-review stage. Having said that, feel free to close this task and we'll reopen/followup when needed.

kk

• chasemp moved this task from Incoming to Our Part Is Done on the Security-Team board.Jan 8 2020, 5:15 PM
fgiunchedi added a comment.Jan 13 2020, 10:44 AM
In T232820#5783328, @chasemp wrote:
In T232820#5744696, @fgiunchedi wrote:
In T232820#5732793, @sbassett wrote:

@fgiunchedi - any update or progress on this? If not, we might just want to decline this task for now until there's actually some code ready for review. Thanks.

Yes, progress in the form of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/553376 which is the client-side javascript part. The code is being reviewed and worked on although I don't think it is yet at the security-review stage. Having said that, feel free to close this task and we'll reopen/followup when needed.

AFAICT the code is basically ready to be merged, when does security review need to happen ?

sbassett added a subscriber: Reedy.EditedJan 13 2020, 10:31 PM
In T232820#5796753, @fgiunchedi wrote:

AFAICT the code is basically ready to be merged, when does security review need to happen ?

@fgiunchedi - a few points:

  1. Given that the patch set is a config file change and some fairy minimal JavaScript for error-tracking, this probably wouldn't warrant a full security readiness review. Those are typically performed for new (or substantial rewrites) of MediaWiki extensions, services, etc. I know our documentation isn't very clear on various review thresholds - hopefully we can improve that at some point.
  2. I'd imagine the Performance-Team might be interested in having a look at this if they haven't already.
  3. Prior to any patch review, we'd probably require that all of the relevant CI checks are passing.
  4. For the patch review itself, @Reedy or I would most likely focus on ensuring that the regexWebkit and regexGecko patterns are sufficiently hardened and if any potential attack vectors against EventGate are being introduced.
fgiunchedi added a comment.Jan 14 2020, 10:22 AM
In T232820#5799861, @sbassett wrote:
In T232820#5796753, @fgiunchedi wrote:

AFAICT the code is basically ready to be merged, when does security review need to happen ?

@fgiunchedi - a few points:

  1. Given that the patch set is a config file change and some fairy minimal JavaScript for error-tracking, this probably wouldn't warrant a full security readiness review. Those are typically performed for new (or substantial rewrites) of MediaWiki extensions, services, etc. I know our documentation isn't very clear on various review thresholds - hopefully we can improve that at some point.
  2. I'd imagine the Performance-Team might be interested in having a look at this if they haven't already.
  3. Prior to any patch review, we'd probably require that all of the relevant CI checks are passing.
  4. For the patch review itself, @Reedy or I would most likely focus on ensuring that the regexWebkit and regexGecko patterns are sufficiently hardened and if any potential attack vectors against EventGate are being introduced.

Thanks for the feedback @sbassett, all good points! Since indeed the code turned out to be simpler than we initially expected no full security review seems needed. I'll ping you and/or @Reedy once the other points have been addressed

sbassett added a comment.Jan 14 2020, 4:28 PM
In T232820#5801191, @fgiunchedi wrote:

Thanks for the feedback @sbassett, all good points! Since indeed the code turned out to be simpler than we initially expected no full security review seems needed. I'll ping you and/or @Reedy once the other points have been addressed

Sounds good. Feel free to follow up on this task, if needed, even though it's already resolved and the code won't require a full security readiness review.

• chasemp added a project: secscrum.Mar 10 2020, 8:17 PM
• chasemp moved this task from Incoming to Our Part Is Done on the secscrum board.Mar 10 2020, 8:18 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL