Page MenuHomePhabricator

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

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: [WIP] [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

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 13 2019, 10:23 AM
sbassett triaged this task as Lowest priority.Sep 17 2019, 4:53 PM

@fgiunchedi - Thanks for submitting this. We'll keep this in our Security Readiness 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

@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 - 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 closed this task as Resolved.Jan 7 2020, 6:54 PM
chasemp claimed this task.
chasemp added a subscriber: chasemp.

@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

@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

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.

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

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.