In T162181#3426331, @Tgr wrote:

@dpatrick thanks for the detailed review! It is somewhat scary, yeah, but this is a command-line tool used by already trusted users so the attack surface will be very limited.

Out of curiosity, do you notify the maintainers of external projects in such cases? For smallish projects it might be helpful information to know that someone who does security for a job did a review.

In T148567#3780384, @mobrovac wrote:

In T148567#3778518, @dpatrick wrote:

Just following up on some lingering security reviews. I know that this service has been deployed. Do we have appropriate firejail and iptables rules in place now to restrict egress?

The service cannot establish any connections outside of the production environment because it does not contact the production proxy, so any attempt to request an external resource will simply time out.

I like this idea, as Moritz laid it out above. I think this would make sense moving forward, and fits well with the release improvment project, as releases could be signed by the Jenkins instance.

Approved!

Many apologies for the delay here. I reviewed this back in June, failed to add my notes, then re-reviewed last week due to code changes since the last time I looked at it. I found no issues while reviewing this library. I checked for the following:

• XSS via unescaped input or failure to maintain escaping (via mustache interpolation, v-model, static data, etc.)
• Resource consumption/DoS
• Template expression injection at runtime from user-controlled data

Just following up on some lingering security reviews. I know that this service has been deployed. Do we have appropriate firejail and iptables rules in place now to restrict egress?

In T178077#3760654, @bmansurov wrote:

will you be using HTMLParser, or an external parser (lxml, html5lib, etc.)?

We'll be using the Python 3's default html.parser for now: https://github.com/kodchi/ppg/blob/master/src/process_toc.py#L26

In T178077#3723899, @phuedx wrote:

FTR this is lower priority than T177765: Security review of mediawiki-services-chromium-render.

This review is complete. Basic concerns such as system i/o (very limited) and shell execution (none) were found to be safe. Encryption implementation was not reviewed since we won't be using that portion of the code. My main concerns would lie with code execution or denial of service (a la https://github.com/pmaupin/pdfrw/issues/92) via malicious PDF input, however, the fact that this will run in a closed ecosystem (in which input originates only from a system we control) mitigates those concerns.

I checked out the WIP ppg code in the description of T171960 and I'm wondering whether that will be invoked by the Node service (T177765), returning a ready-to-read PDF which has ToC, page numbers, etc., or will the Node service just render an article which will then be post-processed (adding ToC, page numbers, etc.) separately? I'm asking to ascertain whether the script which will use pdfrw will be firejailed. This question is not a blocker. I'm just curious.

In T174388#3681713, @APalmer_WMF wrote:

Approved by Legal.

@phuedx, do you mind updating the description to note why Electron needs to be replaced and what problems have been observed? Thanks!

Displaying stacktraces/detailed error messages is generally considered an insecure deployment pattern in web application security. I think I understand the logic for wanting to do so, however I don't support it, despite our redaction of arguments. My concern is that that redaction may somehow fail in a critical way, resulting in unintentional exposure of data beyond that which can be gathered by nature of the openness of our project.

In T173014#3573827, @bmansurov wrote:

In addition to pdfrw, it's looking increasingly likely that we're going to have to use BeautifulSoup for easy DOM querying and manipulation. At this time we won't be using any external parsers such as lxml, but we'll use Python's built in html.parser. Should I create a new task for this? Not sure if any past projects have used this library before, but ORES or Wikimetrics don't seem to use it.

Discussed on 2017-10-04 and approved.

Discussed on 2017-10-04 and approved.

In T174126#3608229, @Fjalapeno wrote:

@dpatrick is this on your radar?

Please see my previous comment, just trying to get this in before end of quarter. Thanks!

In T174489#3568736, @K4-713 wrote:

@dpatrick : Yes, I'm using google authenticator.

Approved.

@K4-713, you will need to have two factor enabled for Phabricator. Can you verify that it is enabled?

@kaldari, I spoke with @Reedy and put this on the schedule for the week of 8/28. Sam's already started working on it.

FWIW, I support stating clearly at sign-up time that origin IP addresses are not private when using labs/toolforge. I don't believe we have the resources to fully lockdown all mechanisms of accessing this information, as @bd808 mentions above.

@DStrine, can mitigation work for this issue be added to the workboard for @AndyRussG and @awight?

Sorry for the delay @Jalexander. The team discussed and approved this weeks ago, but I forgot say son on the ticket. Approved!